-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add Prepare and Process Proposal ABCI++ methods #631
Conversation
Still unsure as to why the test_apps script is failing. We've had trouble with it in the past, so I'm not convinced that it is a deal breaker for merging this PR as long as both the e2e and app's integration test are passing. This test will likely be fixed when we use a stable version of ABCI++. |
tbc, this PR is a placeholder that allows us to start building features in the app that depend upon these two added methods. When the release version of the cosmos-sdk that uses tendermint v0.36.x comes out, then we can create a version of celestia-core that is based on v0.36.x and use that instead |
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.
This is pretty cool! Looking forward to testing this with the SDK/our app.
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.
Left one nit. The changes here are straightforwardly pulling from upstream.
I think it would be important to clarify in the opening comment from which branch(es)/commit(s)/version(s) this was pulled exactly.
Similarly:
Modifies both methods to work on tendermint v0.34.x
It would be good if the PR opening comment documented what modifications were necessary here.
Co-authored-by: Ismail Khoffi <[email protected]>
Co-authored-by: John Adler <[email protected]>
Description
This PR incorporates the two needed ABCI++ methods from upstream.
types.Data
instead oftypes.Txs
or[][]byte
Closes #623
related to #157