Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CHIA-1348] Port chia wallet coin commands to @chia_command framework #18641

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Quexington
Copy link
Contributor

This PR takes the first step in porting existing wallet commands to the @chia_command framework currently in use for the signer commands. The primary benefit of this framework is that we can start writing integration tests with the command line interface at the top instead of the RPC so we can actually test the entire stack that a user utilizes when interacting with our application. It also comes with strong typing for CLI parameters and better code unity (right now the click options and the actual execution code live in separate places).

The coin commands were chosen due to the smaller number of them and the recent attention that has been paid to their tests making it easy to port the commands right on top of those recent changes.

@Quexington Quexington added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Sep 25, 2024
@Quexington Quexington changed the title Port chia wallet coin commands to @chia_command framework [CHIA-1348] Port chia wallet coin commands to @chia_command framework Sep 25, 2024
@Chia-Network Chia-Network deleted a comment from coveralls-official bot Sep 26, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Sep 26, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Sep 27, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 3, 2024
@github-actions github-actions bot removed merge_conflict Branch has conflicts that prevent merge to main labels Oct 28, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Oct 28, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Oct 28, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Oct 28, 2024
@@ -20,7 +20,7 @@ def test_cmd(**kwargs: Any) -> list[TransactionRecord]:

runner: CliRunner = CliRunner()
with runner.isolated_filesystem():
runner.invoke(test_cmd, ["--transaction-file", "./temp.transaction"])
runner.invoke(test_cmd, ["--transaction-file-out", "./temp.transaction"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am aware this is a command line API change, but a sort of necessary one from an initial bad decision that I think will cause minimal headache.

@command_helper
class TransactionsIn:
transaction_file_in: str = option(
"--transaction-file-in",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is an API change (there used to be a short -i option) but I think this will cause even less headache than the other API change.

@Quexington Quexington marked this pull request as ready for review October 28, 2024 21:22
@Quexington Quexington requested a review from a team as a code owner October 28, 2024 21:22
@emlowe
Copy link
Contributor

emlowe commented Nov 7, 2024

One could argue that the RPC commands should be tested independently of the CLI commands. Datalayer takes this approach and tests the CLI, the RPC api, the RPC client and the underlying functions where possible for each test.

@Quexington
Copy link
Contributor Author

Quexington commented Nov 7, 2024

One could argue that the RPC commands should be tested independently of the CLI commands. Datalayer takes this approach and tests the CLI, the RPC api, the RPC client and the underlying functions where possible for each test.

One could certainly argue that yeah. From my perspective, this isn't actually asserting an opinion one way or another on that because we don't test at every layer right now and so this just makes it ergonomic enough to test all the way from (mostly) the top. If you wanted to test at every layer, this PR would still be a dependency in order to test at the CLI layer.

Also, sort of unrelated, I hope one day that it does not make sense to test at every layer and instead to elevate the user request -> state change relationship to its own test and to separately test that every piece of plumbing in between can handle passing thru any user request.

@altendky
Copy link
Contributor

For what it's worth, I'm on board with the idea that each layer should become more and more systematic. This means you would want to test the system/framework at each layer but not every bit of every use at each layer. There would be a common set of types that each layer must support and a definition of the abilities that each layer would need to implement. Now, interfaces that are good for an RPC aren't always best for a CLI, so we may hit some snags to deal with.

Also, the testing pattern you referenced that I wrote was a messy exploration. I like the idea. I'm not encouraging propagating exactly it everywhere. And, the more systematic each layer gets the more we can write a test case and have it 'generate' tests for each layer's inputs and outputs individually and also through the whole stack.

But... I need to look back at this PR and get familiar with it. I set it aside for far too long. Sorry. :[

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 14, 2024
@github-actions github-actions bot removed merge_conflict Branch has conflicts that prevent merge to main labels Nov 15, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Nov 15, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Nov 15, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants