-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
abci-cli: added PrepareProposal
command to cli
#8656
Conversation
abci/cmd/abci-cli/abci-cli.go
Outdated
@@ -333,6 +342,13 @@ func cmdTest(cmd *cobra.Command, args []string) error { | |||
}, nil) | |||
}, | |||
func() error { return servertest.Commit(ctx, client, []byte{0, 0, 0, 0, 0, 0, 0, 5}) }, | |||
func() error { | |||
return servertest.PrepareProposal(ctx, client, [][]byte{ | |||
[]byte("abc"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why abc
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed to a random transaction as this should technically pass the prepare proposal. But I have issues with these tests written the way they are.. These are app specific things and it is unclear whether any of these should pass or not by default.
PrepareProposal
command to cli #8643PrepareProposal
command to cli #8643
@@ -600,6 +618,43 @@ func cmdQuery(cmd *cobra.Command, args []string) error { | |||
return nil | |||
} | |||
|
|||
func cmdPrepareProposal(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might also be useful to accept extension... although I guess the complexity wouldn't justify the usefulness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergio-mena I agree, but I think we would want to rewrite a lot of these tests at some point but right now this is not very high on the priority list. Can you please re-review the code that there is now, is it sufficient to provide basic testing functionality for someone wanting to use the command line for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look
PrepareProposal
command to cli #8643PrepareProposal
command to cli
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
* [cherrypicked] abci-cli: added `PrepareProposal` command to cli (#8656) * Prepare prosal cli * [cherrypicked + fixes] abci-cli: Add `process_proposal` command to abci-cli (#8901) * Add `process_proposal` command to abci-cli * Added process proposal to the 'tutorial' examples * Added entry in CHANGELOG_PENDING.md * Allow empty blocks in PrepareProposal, ProcessProposal, and FinalizeBlock * Fix minimum arguments * Add tests for empty block * Updated abci-cli doc Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Addressed @williambanfield's comment Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> Co-authored-by: Hernán Vanzetto <hernan.vanzetto@gmail.com>
Closes #8643 .
Added cli command for PrepareProposal.