osmosis
osmosis copied to clipboard
feat: sdk v0.50.x upgrade
Closes: #XXX
What is the purpose of the change
References
SDK Upgrading Guide: https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#v050x DYDX Upgrade PRs:
- https://github.com/dydxprotocol/v4-chain/pull/867
- https://github.com/dydxprotocol/v4-chain/pull/932
Notes
- Unclear what this line in the upgrading.md is talking about https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#stringer. As in, do we need to ensure .String() isn't used anywhere in the state machine now??
- Do we continue using osmomath types for math types or do we replace all of them with the math package https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#math
- Did not set up vote extensions https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#baseapp
- Did not set up textual sign mode https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#textual-sign-mode
- Ensure messages that still implement validate basic still call validate basic, despite validate basic being deprecated.
Testing and Verifying
Keeping track here of TODOs that will likely transcend this PR:
- [ ] Check on ante CheckIfBlocked, smartaccount AnteHandle, smartaccount ValidateAuthenticatorFeePayer, smartacount GenerateAuthenticationRequest, smartaccount PostHandle. These all use GetMsgV1Signers to determine the signers, since GetSigners() was removed.
Documentation and Release Note
- [ ] Does this pull request introduce a new feature or user-facing behavior changes?
- [ ] Changelog entry added to
Unreleasedsection ofCHANGELOG.md?
Where is the change documented?
- [ ] Specification (
x/{module}/README.md) - [ ] Osmosis documentation site
- [ ] Code comments?
- [ ] N/A
Important Notice
This PR includes modifications to the tests/e2e/initialization module.
Please follow the instructions below:
- Backport these changes to the previous Osmosis version's branch.
- Run the script inside a Docker container to update genesis and configs for pre-upgrade Osmosis.
- Merge the backported changes.
- The image will be built and uploaded to Docker Hub here.
- Grab the latest image and update it in the PR to the main branch replacing the
previousVersionInitTagin theosmosis/tests/e2e/containers/config.go
Please let us know if you need any help.
Unclear what this line in the upgrading.md is talking about https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#stringer. As in, do we need to ensure .String() isn't used anywhere in the state machine now??
I think we should just review if there are state-machine usages. If so, look at the scope of the modified methods
Do we continue using osmomath types for math types or do we replace all of them with the math package https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#math
I think we can continue using osmomath as is. I think what they refer to is different from the purpose of our osmomath aliasing which was made to avoid the verbosity of LegacyDec. Additionally, we often need to import both BigDec and other sdk math types. Having a single math import makes a light but meaningful cognitive overhead reduction.
In any way, removing aliases is a state-compatible change which we can do at any time if it's ever needed
I think we should just review if there are state-machine usages. If so, look at the scope of the modified methods
Maybe I am not understanding, but don't we use it essentially everywhere in a stateful manner? Here is a random example:
https://github.com/osmosis-labs/osmosis/blob/7c213f8d2f577ffd15964adc6b9de6785208d563/x/tokenfactory/keeper/before_send.go#L121-L122
Merging, with the understanding we have to address all outstanding unforking v2 TODOs prior to release.
Thanks all for the reviews!