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

Correctly handle multiple passed upgrade proposals acc to cosmos-sdk #24

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

sin3point14
Copy link
Contributor

If multiple passed upgrade proposals are in the "passed" state, the cosmos upgrade handler only treats the one with the highest proposal ID as "passed" and all other passed proposals as "cancelled".

ref: https://github.com/cosmos/cosmos-sdk/blob/f007a4ea0711da2bac20afc6283885c1b2496ae5/x/upgrade/keeper/keeper.go#L189-L193.
Thanks to the zetachain team for discovering this.

Comment on lines -69 to +90
newProposal(t, 3, 200, v1.StatusPassed),
newProposal(t, 3, 200, v1.StatusDepositPeriod),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to change the status here as the newly added logic would cancel the proposal at height 100 seeing a passed upgrade at 200, which is the correct behaviour.

If multiple passed upgrade proposals are in the "passed" state,
the cosmos upgrade handler only treats the one with the highest proposal ID
as "passed" and all other passed proposals as "cancelled".
https://github.com/cosmos/cosmos-sdk/blob/f007a4ea0711da2bac20afc6283885c1b2496ae5/x/upgrade/keeper/keeper.go#L189-L193
@sin3point14 sin3point14 force-pushed the multiple-passed-props branch from 4ac9478 to e389757 Compare November 29, 2024 08:00
Copy link
Contributor

@mkaczanowski mkaczanowski left a comment

Choose a reason for hiding this comment

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

LGTM

@sin3point14 sin3point14 merged commit a2c9d9b into main Nov 29, 2024
2 checks passed
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