feat!: port LSM module to SDK v0.47.x
#18694
Conversation
* Format protos * Fix Proto lints * update protos * make proto-gen * implement necessary interfaces * remove minSelfDelegation (partially) --------- Co-authored-by: Zaki Manian <[email protected]>
* set min_self_delegation to 0 in TestAminoCodecFullDecodeAndEncode * add WithdrawTokenizeShareRecordReward and WithdrawAllTokenizeShareRecordReward * add methods to distribution/keeper * register distribution msgs * add SimulateMsgWithdrawTokenizeShareRecordReward * LSM distribution queries * LSM distr cli * add BeforeTokenizeShareRecordRemoved hook * add signers to proto distribution * set signers correctly
* set min_self_delegation to 0 in TestAminoCodecFullDecodeAndEncode * add WithdrawTokenizeShareRecordReward and WithdrawAllTokenizeShareRecordReward * add methods to distribution/keeper * register distribution msgs * add SimulateMsgWithdrawTokenizeShareRecordReward * LSM distribution queries * LSM distr cli * add BeforeTokenizeShareRecordRemoved hook * add signers to proto distribution * set signers correctly * minimum refactor to build * tag LSM test to be refactored * Merge with feat/lsm/v0.47.x tag LSM tests to be refactored Fix nits * nit * comments more failing tests * make protos * Update x/staking/keeper/msg_server.go Co-authored-by: Marius Poke <[email protected]> * Update x/staking/keeper/msg_server.go Co-authored-by: Marius Poke <[email protected]> * add go.work and fix silent errors * address comments * tests: add lsm distribution tests (#6) --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: MSalopek <[email protected]>
* set min_self_delegation to 0 in TestAminoCodecFullDecodeAndEncode * add WithdrawTokenizeShareRecordReward and WithdrawAllTokenizeShareRecordReward * add methods to distribution/keeper * register distribution msgs * add SimulateMsgWithdrawTokenizeShareRecordReward * LSM distribution queries * LSM distr cli * add BeforeTokenizeShareRecordRemoved hook * add signers to proto distribution * set signers correctly * minimum refactor to build * tag LSM test to be refactored * Merge with feat/lsm/v0.47.x tag LSM tests to be refactored Fix nits * nit * comments more failing tests * make protos * Update x/staking/keeper/msg_server.go Co-authored-by: Marius Poke <[email protected]> * Update x/staking/keeper/msg_server.go Co-authored-by: Marius Poke <[email protected]> * add go.work and fix silent errors * address comments * refactor staking msg_server_tests.go - distrib hooks cause them to fail * make integration tests pass * clean up * clean up * refactor last integration * nits * revert deterministic tests change * tests: update simulation randfees calc (#9) * address min self delegation depreciation in tests * refactor e2e tests and other nits * tests: appease linter in randfees --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: MSalopek <[email protected]>
sainoe
added
C:x/staking
C:CLI
C:x/slashing
C:x/distribution
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
gosec found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| func (msg MsgWithdrawAllTokenizeShareRecordReward) GetSigners() []sdk.AccAddress { | ||
| owner, err := sdk.AccAddressFromBech32(msg.OwnerAddress) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
||
| tokensBz, err := tokens.Marshal() | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
||
| var tokens math.Int | ||
| if err := tokens.Unmarshal(tokensBz); err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| // This only error's if the total liquid staked tokens underflows | ||
| // which would indicate there's a corrupted state where the validator has | ||
| // liquid tokens that are not accounted for in the global total | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (msg MsgTransferTokenizeShareRecord) GetSigners() []sdk.AccAddress { | ||
| sender, err := sdk.AccAddressFromBech32(msg.Sender) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (msg MsgDisableTokenizeShares) GetSigners() []sdk.AccAddress { | ||
| sender, err := sdk.AccAddressFromBech32(msg.DelegatorAddress) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (msg MsgEnableTokenizeShares) GetSigners() []sdk.AccAddress { | ||
| sender, err := sdk.AccAddressFromBech32(msg.DelegatorAddress) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (msg MsgValidatorBond) GetSigners() []sdk.AccAddress { | ||
| delegator, err := sdk.AccAddressFromBech32(msg.DelegatorAddress) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -17,6 +17,7 @@ | |||
| defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) | |||
|
|
|||
| k.TrackHistoricalInfo(ctx) | |||
| k.RemoveExpiredTokenizeShareLocks(ctx, ctx.BlockTime()) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
Migration code was done in this commit: The same commit is visible in the PR using: |
| MinCommissionRate: sdk.NewDecWithPrec(rapid.Int64Range(0, 100).Draw(t, "commission"), 2), | ||
| ValidatorBondFactor: sdk.NewDecWithPrec(rapid.Int64Range(0, 100).Draw(t, "bond-factor"), 2), | ||
| GlobalLiquidStakingCap: sdk.NewDecWithPrec(rapid.Int64Range(0, 100).Draw(t, "global-liquid-staking-cap"), 2), | ||
| ValidatorLiquidStakingCap: sdk.NewDecWithPrec(rapid.Int64Range(0, 100).Draw(t, "validator-liquid-staking-cap"), 2), |
There was a problem hiding this comment.
We set the validator liquid staking cap to 100% on the cosmos hub. I am not sure this value is necessary
There was a problem hiding this comment.
I agree in the context of the hub. However, I believe we still gain some confidence in the overall logic by varying the ValidatorLiquidStakingCap value in the tests.
|
Seems the |
There was a problem hiding this comment.
Looks great!
Let me know if there are any sensitive areas that warrant a more thorough review (i.e. any place where the changes were more than just a copy+paste). I did a slower/more focused review for the staking msg_server, liquid_stake.go, and the migration.
| // Checks if an account associated with a given delegation is related to liquid staking | ||
| // | ||
| // This is determined by checking if the account has a 32-length address | ||
| // which will identify the following scenarios: | ||
| // - An account has tokenized their shares, and thus the delegation is | ||
| // owned by the tokenize share record module account | ||
| // - A liquid staking provider is delegating through an ICA account | ||
| // | ||
| // Both ICA accounts and tokenize share record module accounts have 32-length addresses | ||
| // NOTE: This will have to be refactored before adapting it to chains beyond gaia | ||
| // as other chains may have 32-length addresses that are not related to the above scenarios | ||
| func (k Keeper) DelegatorIsLiquidStaker(delegatorAddress sdk.AccAddress) bool { | ||
| return len(delegatorAddress) == 32 | ||
| } |
There was a problem hiding this comment.
(this comment shouldn't block the upgrade to 47) but might be worth revisiting what the long term plan is here, wrt to upstreaming this into 51.
Iirc, legacy CW addresses are less than 32 bytes so I don't think this covers all cases on a chain with CW enabled. That said, maybe that doesn't matter if LSPs would have to move off the legacy contract in order to upgrade?
| func (msg MsgUnbondValidator) GetSigners() []sdk.AccAddress { | ||
| valAddr, err := sdk.ValAddressFromBech32(msg.ValidatorAddress) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Not sure it really matters, but I think for the other messages in 47, there's no longer panics here after the address validation
| OpWeightMsgEditValidator = "op_weight_msg_edit_validator" | ||
| OpWeightMsgDelegate = "op_weight_msg_delegate" | ||
| OpWeightMsgUndelegate = "op_weight_msg_undelegate" | ||
| OpWeightMsgBeginRedelegate = "op_weight_msg_begin_redelegate" |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgCreateValidator = "op_weight_msg_create_validator" | ||
| OpWeightMsgEditValidator = "op_weight_msg_edit_validator" | ||
| OpWeightMsgDelegate = "op_weight_msg_delegate" | ||
| OpWeightMsgUndelegate = "op_weight_msg_undelegate" |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
|
|
||
| OpWeightMsgCreateValidator = "op_weight_msg_create_validator" | ||
| OpWeightMsgEditValidator = "op_weight_msg_edit_validator" | ||
| OpWeightMsgDelegate = "op_weight_msg_delegate" |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| DefaultWeightMsgDisableTokenizeShares int = 1 | ||
|
|
||
| OpWeightMsgCreateValidator = "op_weight_msg_create_validator" | ||
| OpWeightMsgEditValidator = "op_weight_msg_edit_validator" |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| DefaultWeightMsgFundCommunityPool int = 50 | ||
| OpWeightMsgSetWithdrawAddress = "op_weight_msg_set_withdraw_address" //nolint:gosec | ||
| OpWeightMsgWithdrawDelegationReward = "op_weight_msg_withdraw_delegation_reward" //nolint:gosec | ||
| OpWeightMsgWithdrawValidatorCommission = "op_weight_msg_withdraw_validator_commission" //nolint:gosec |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgBeginRedelegate = "op_weight_msg_begin_redelegate" | ||
| OpWeightMsgCancelUnbondingDelegation = "op_weight_msg_cancel_unbonding_delegation" | ||
| OpWeightMsgValidatorBond = "op_weight_msg_validator_bond" //nolint:gosec | ||
| OpWeightMsgTokenizeShares = "op_weight_msg_tokenize_shares" //nolint:gosec |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgUndelegate = "op_weight_msg_undelegate" | ||
| OpWeightMsgBeginRedelegate = "op_weight_msg_begin_redelegate" | ||
| OpWeightMsgCancelUnbondingDelegation = "op_weight_msg_cancel_unbonding_delegation" | ||
| OpWeightMsgValidatorBond = "op_weight_msg_validator_bond" //nolint:gosec |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgDelegate = "op_weight_msg_delegate" | ||
| OpWeightMsgUndelegate = "op_weight_msg_undelegate" | ||
| OpWeightMsgBeginRedelegate = "op_weight_msg_begin_redelegate" | ||
| OpWeightMsgCancelUnbondingDelegation = "op_weight_msg_cancel_unbonding_delegation" |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgWithdrawDelegationReward = "op_weight_msg_withdraw_delegation_reward" //nolint:gosec | ||
| OpWeightMsgWithdrawValidatorCommission = "op_weight_msg_withdraw_validator_commission" //nolint:gosec | ||
| OpWeightMsgFundCommunityPool = "op_weight_msg_fund_community_pool" //nolint:gosec | ||
| OpWeightMsgWithdrawAllTokenizeShareRecordReward = "op_weight_msg_withdraw_all_tokenize_share_record_reward" //nolint:gosec |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
| OpWeightMsgSetWithdrawAddress = "op_weight_msg_set_withdraw_address" //nolint:gosec | ||
| OpWeightMsgWithdrawDelegationReward = "op_weight_msg_withdraw_delegation_reward" //nolint:gosec | ||
| OpWeightMsgWithdrawValidatorCommission = "op_weight_msg_withdraw_validator_commission" //nolint:gosec | ||
| OpWeightMsgFundCommunityPool = "op_weight_msg_fund_community_pool" //nolint:gosec |
Check failure
Code scanning / gosec
Potential hardcoded credentials Error
* Fix bug in `CheckExceedsValidatorLiquidStakingCap` formula * Update x/staking/keeper/liquid_stake.go Co-authored-by: Marius Poke <[email protected]> * fix unit test bug --------- Co-authored-by: Marius Poke <[email protected]>
* udapte stakind README + ADR * nits
sainoe
merged commit 173f2ea
into
cosmos:feature/v0.47.x-ics-lsm
Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Zaki Manian <[email protected]> Co-authored-by: MSalopek <[email protected]>
Description
Port the LSM module changes from feature/v0.45.x-ics-lsm to feature/v0.47.x-ics-lsm, which is pointing at
release/v0.47.x@044ff4d8c015.Closes: #18200
cc: @riley-stride, @sampocs
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...