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: cosmos-sdk v0.45.3 upgrade ver #68

Closed

Conversation

0xHansLee
Copy link

The branch v0.45.3-panacea is from v0.45.3 tag of cosmos-sdk.
As done in #27, the minimum commission rate has been added to x/staking module.

What I've done in this PR:

  • add min_commission_rate to the x/staking module
  • remove the MustUpdateValidatorCommission() func since it is no longer needed.
  • in x/staking/legacy/v040/migrate.go, the old staking.pb.go file is moved inside the legacy/v040 folder and it is used for migration. But, we need to migrate with min_commission_rate, which is not included the old staking.pb.go file, so I changed to use the generated staking.pb.go file (v040staking.*)

Copy link

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

Thank you!
I have commented on a few questions.

Comment on lines +7601 to +7603
| SIGN_MODE_EIP_191 | 191 | SIGN_MODE_EIP_191 specifies the sign mode for EIP 191 signing on the Cosmos SDK. Ref: https://eips.ethereum.org/EIPS/eip-191

Currently, SIGN_MODE_EIP_191 is registered as a SignMode enum variant, but is not implemented on the SDK by default. To enable EIP-191, you need to pass a custom `TxConfig` that has an implementation of `SignModeHandler` for EIP-191. The SDK may decide to fully support EIP-191 in the future.
Copy link

Choose a reason for hiding this comment

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

I wonder where this SIGN_MODE_EIP_191 was found.
Because the documentation for cosmos-sdk version 0.45.3 doesn't include this. :)

Copy link
Author

Choose a reason for hiding this comment

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

You're right. It is included in v0.45.5. It seems that it's tangled when checking out branches.
BTW, I'm preparing new PR for v0.45.4. Because the test of resetta is failed due to the version of go, which is required 1.17 or upper ver. It was fixed in v0.45.4.. After I test locally, I'll make a new PR.
Thanks for comments!

Copy link

Choose a reason for hiding this comment

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

Aha. OK.

@@ -78,8 +85,9 @@ func RandomizedGenState(simState *module.SimulationState) {
valAddrs[i] = valAddr

maxCommission := sdk.NewDecWithPrec(int64(simulation.RandIntBetween(simState.Rand, 1, 100)), 2)
curCommissionRate := simulation.RandomDecAmount(simState.Rand, maxCommission.Sub(minComRate)).Add(minComRate)
Copy link

Choose a reason for hiding this comment

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

Why did you make this change?

Copy link
Author

Choose a reason for hiding this comment

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

I copied from v0.42.11-panacea branch (

curCommissionRate := simulation.RandomDecAmount(simState.Rand, maxCommission.Sub(minComRate)).Add(minComRate)
)

Copy link

Choose a reason for hiding this comment

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

Ah, that's right.
It is not important, but in the current test, if the value of min commission is not 0% (if it exceeds 1%), curCommissionRate may come out negative.
But as I mentioned above, it doesn't matter. :)

Copy link

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

thank you

@0xHansLee
Copy link
Author

@gyuguen, @youngjoon-lee
Thank you for your review and sorry to bother you.
The reason why the tests(test-rosetta, liveness-test) failed in this PR can be passed with go ver 1.17 or upper.
And it is fixed in v0.45.4, so I will close this PR and make another PR for upgrade to v0.45.4. Hope all tests will pass.

@0xHansLee 0xHansLee closed this Jun 22, 2022
@youngjoon-lee
Copy link

thank u. it’s good enough to use v0.45.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants