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

Add Prepare and Process Proposal ABCI++ methods #631

Merged
merged 19 commits into from
Mar 30, 2022

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Feb 16, 2022

Description

This PR incorporates the two needed ABCI++ methods from upstream.

  • cherry-picks the PrepareProposal and ProcessProposal ABCI++ methods
  • Modifies both methods to work on tendermint v0.34.x
    • This includes minor changes such as removing context usuage and returning errors, along with moving code outside of interanlized packages.
  • Modifies both methods to use types.Data instead of types.Txs or [][]byte

Closes #623

related to #157

@evan-forbes evan-forbes added the C:abci The connection between ll-core and the (abci) app label Feb 16, 2022
@evan-forbes evan-forbes self-assigned this Feb 16, 2022
@evan-forbes evan-forbes removed the request for review from tac0turtle February 16, 2022 06:21
@evan-forbes
Copy link
Member Author

evan-forbes commented Feb 16, 2022

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++.

@evan-forbes
Copy link
Member Author

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

Copy link
Member

@liamsi liamsi left a 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.

proto/tendermint/abci/types.proto Outdated Show resolved Hide resolved
state/mocks/evidence_pool.go Outdated Show resolved Hide resolved
types/tx.go Outdated Show resolved Hide resolved
Copy link
Member

@liamsi liamsi left a 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.

abci/types/application.go Outdated Show resolved Hide resolved
abci/types/application.go Outdated Show resolved Hide resolved
@evan-forbes evan-forbes merged commit 7356306 into v0.34.x-celestia Mar 30, 2022
@evan-forbes evan-forbes deleted the evan/Prepare-Process-Proposal branch March 30, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci The connection between ll-core and the (abci) app
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants