LSM Migration for v14 #910
Conversation
sampocs
commented
Aug 25, 2023
•
sampocs
commented
- Migrated stakeibc host zone and validator types
- Migrated stakeibc params
- Cleared out pending queries
- Enabled LSM gaia
x/stakeibc/migrations/v3/convert.go
Outdated
| ValidatorSlashQueryThreshold uint64 = 1 | ||
| // The exchange rate here does not matter since it will be updated after the slash query | ||
| // Setting it to this value makes it easier to verify that we've submitted the query | ||
| DefaultExchangeRate = sdk.MustNewDecFromStr("1.000000000000001") |
There was a problem hiding this comment.
Won't this rate be used in the RR if we hit an epoch before a first slash query to this validator?
There was a problem hiding this comment.
(Separate) nit: I think exchRate g.t. 1 is impossible so might make sense to make the default (even if unused) something like 0.999 instead.
There was a problem hiding this comment.
It's only used in the redemption rate when summing up LSMTokenDeposits, but we should run the query on all validators before LSM is even live on the hub
There was a problem hiding this comment.
also, agreed, 0.9999 probably makes more sense
There was a problem hiding this comment.
Cool this makes sense
|
Quick review on the convert functions. Added some comments. Going to review the rest of the PR now. |
| // - Added SlashQueryInProgress field | ||
| // - InternalExchangeRate is now a decimal named SharesToTokensRate | ||
| // - DelegationAmt renamed to Delegation | ||
| func convertToNewValidator(oldValidator oldstakeibctypes.Validator, totalDelegations sdkmath.Int) newstakeibctypes.Validator { |
There was a problem hiding this comment.
Looks good once the questions re DefaultExchangeRate are addressed.
Confirming that you haven't missed any proto fields on Validator.
| TransferChannelId: oldHostZone.TransferChannelId, | ||
| IbcDenom: oldHostZone.IbcDenom, | ||
| HostDenom: oldHostZone.HostDenom, | ||
| UnbondingPeriod: (oldHostZone.UnbondingFrequency - 1) * 7, |
There was a problem hiding this comment.
This arithmetic seems right, but given how sensitive it is and the fact that we've debated the formula in the past (though I think we're all aligned on it now) let's make sure we are all in sync on this before merging.
There was a problem hiding this comment.
Verified with all the existing host zones that this formula lines up
x/stakeibc/migrations/v3/convert.go
Outdated
| ValidatorSlashQueryThreshold uint64 = 1 | ||
| // The exchange rate here does not matter since it will be updated after the slash query | ||
| // Setting it to this value makes it easier to verify that we've submitted the query | ||
| DefaultExchangeRate = sdk.MustNewDecFromStr("1.000000000000001") |
There was a problem hiding this comment.
Cool this makes sense
x/stakeibc/keeper/host_zone.go
Outdated
| // TODO: Revert after upgrade v14 | ||
| // return errorsmod.Wrapf(types.ErrInvalidValidatorDelegationUpdates, | ||
| // "cannot decrement the number of delegation updates if the validator has 0 updates in progress") | ||
| return nil |
There was a problem hiding this comment.
As I understand it this is the 0 floor. In theory this line should not be reachable, so imo we should emit an event here and log before returning nil, so that we can trace any issues. Then in v14 we revert.
There was a problem hiding this comment.
good call, added a log
