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

sdk v50 proposal compatibility #840

Merged
merged 6 commits into from
Nov 7, 2023
Merged

sdk v50 proposal compatibility #840

merged 6 commits into from
Nov 7, 2023

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Oct 24, 2023

(used in x/POA module)

SDK v50 added more governance params in the proposal.json.

The new functions are suffixed with the IBC Go / interchaintest version (ex: BuildProposal and BuildProposalV8). Should we instead prefix with SDK versions?

after merge todo

  • tag v7 and v8 releases

@Reecepbcups
Copy link
Member Author

Reecepbcups commented Nov 2, 2023

Ahhh we need to be backwards compatibility for SDK v47 (v7) ->v50 (v8) upgrades

So multiple structs will be required. Suffix based off the IBC go version instead of SDK

@Reecepbcups Reecepbcups changed the title SDK v50 - Proposal sdk v50 proposal compatibility Nov 7, 2023
@Reecepbcups Reecepbcups marked this pull request as ready for review November 7, 2023 16:41
@Reecepbcups Reecepbcups requested a review from a team as a code owner November 7, 2023 16:41
@Reecepbcups Reecepbcups requested a review from boojamya November 7, 2023 16:41
@Reecepbcups Reecepbcups enabled auto-merge (squash) November 7, 2023 16:41
Copy link
Contributor

@boojamya boojamya left a comment

Choose a reason for hiding this comment

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

Nice, looks good!
One nit. Approving to not block.

@@ -439,7 +444,9 @@ func (c *CosmosChain) SubmitProposal(ctx context.Context, keyName string, prop T
}

// Build a gov v1 proposal type.
func (c *CosmosChain) BuildProposal(messages []cosmosproto.Message, title, summary, metadata, depositStr string) (TxProposalv1, error) {
//
// The proposer field should only be set for IBC-Go v8 / SDK v50 chains.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add some sort of note that the expedited field is also not ingested for non-sdk50 chains.

Copy link
Member Author

@Reecepbcups Reecepbcups Nov 7, 2023

Choose a reason for hiding this comment

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

this will not matter so long as proposer is blank. Will error out if they try anything for legacy chains

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait how does depositStr come into play here?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops! sorry I mis-copied proposer

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy that. It was more of a guidance thing for users. But yea, it's self explanatory, should be fine.

@Reecepbcups Reecepbcups merged commit cc09872 into main Nov 7, 2023
11 checks passed
@Reecepbcups Reecepbcups deleted the reece/sdk-50-gov branch November 7, 2023 18:58
@Reecepbcups Reecepbcups removed their assignment Jun 27, 2024
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.

2 participants