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

feat: pass chain-id in prepareProposal #1028

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Jul 4, 2023

The cosmos-sdk doesn't have a good way to access the chain-id from the app atm, so we're passing via prepareProposal instead

closes #1027
part of fixing celestiaorg/celestia-app#2020

@evan-forbes evan-forbes self-assigned this Jul 4, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Jul 4, 2023
Comment on lines -214 to +218
repeated bytes data = 1;
repeated NMTProof share_proofs = 2;
bytes namespace_id = 3;
RowProof row_proof = 4;
uint32 namespace_version = 5;
repeated bytes data = 1;
repeated NMTProof share_proofs = 2;
bytes namespace_id = 3;
RowProof row_proof = 4;
uint32 namespace_version = 5;
Copy link
Member Author

Choose a reason for hiding this comment

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

called proto lint + format, not sure why our protobuf linter doesn't also call those

@evan-forbes
Copy link
Member Author

my protobuf setup adds an extra line \n in a few files relative to the CI, hence the protobuf generation failure.

either we can fix later, or someone with an identical setup to the CI can push to fix, but I don't think we should block on that given we need to ship asap

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

either we can fix later, or someone with an identical setup to the CI can push to fix

  1. Can you open an issue to look into this
  2. Can you just manually edit the files to match? Otherwise all subsequent PRs will also have this check broken and actual bugs might slip through!

@evan-forbes
Copy link
Member Author

Can you just manually edit the files to match? Otherwise all subsequent PRs will also have this check broken and actual bugs might slip through!

@sweexordious has a setup identical to CI, so this has fixed it. In the meantime I'll try and fix my local setup, or update the CI

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

one non blocking question about the godoc

@@ -133,6 +133,8 @@ message RequestPrepareProposal {
tendermint.types.Data block_data = 1;
// If an application decides to populate block_data with extra information, they can not exceed this value.
int64 block_data_size = 2;
// proposal_header is the header from the proposal
Copy link
Collaborator

@rootulp rootulp Jul 4, 2023

Choose a reason for hiding this comment

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

[question] is this godoc copied from somewhere? If not, proposal to make the godoc reference the field name b/c I'm not sure what the relationship is between proposal_header and chain_id but they seem distinct.

Suggested change
// proposal_header is the header from the proposal
// chain_id is a unique identifier for the blockchain network this proposal belongs to (e.g. mocha-1)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create an issue for this one, as changing will require recompiling the proto and the CI and my setup and different somehow despite using the same versions...

Copy link
Member Author

Choose a reason for hiding this comment

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

@evan-forbes evan-forbes merged commit 67cc27b into v0.34.x-celestia Jul 4, 2023
@evan-forbes evan-forbes deleted the evan/pass-header-prepare branch July 4, 2023 13:41
cmwaters pushed a commit that referenced this pull request Jul 20, 2023
* feat: pass chain-id in prepareProposal

* chore: proto-gen

---------

Co-authored-by: rachid <chamirachid1@gmail.com>
cmwaters pushed a commit that referenced this pull request Jul 24, 2023
* feat: pass chain-id in prepareProposal

* chore: proto-gen

---------

Co-authored-by: rachid <chamirachid1@gmail.com>
@faddat faddat mentioned this pull request Feb 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass ChainID via RequestPrepareProposal
4 participants