osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

feat: sdk v0.50.x upgrade

Open czarcas7ic opened this issue 1 year ago • 4 comments

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 Unreleased section of CHANGELOG.md?

Where is the change documented?

  • [ ] Specification (x/{module}/README.md)
  • [ ] Osmosis documentation site
  • [ ] Code comments?
  • [ ] N/A

czarcas7ic avatar May 17 '24 00:05 czarcas7ic

Important Notice

This PR includes modifications to the tests/e2e/initialization module. Please follow the instructions below:

  1. Backport these changes to the previous Osmosis version's branch.
  2. Run the script inside a Docker container to update genesis and configs for pre-upgrade Osmosis.
  3. Merge the backported changes.
  4. The image will be built and uploaded to Docker Hub here.
  5. Grab the latest image and update it in the PR to the main branch replacing the previousVersionInitTag in the osmosis/tests/e2e/containers/config.go

Please let us know if you need any help.

github-actions[bot] avatar May 17 '24 01:05 github-actions[bot]

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

p0mvn avatar May 21 '24 01:05 p0mvn

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

p0mvn avatar May 21 '24 01:05 p0mvn

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

czarcas7ic avatar May 21 '24 20:05 czarcas7ic

Merging, with the understanding we have to address all outstanding unforking v2 TODOs prior to release.

Thanks all for the reviews!

czarcas7ic avatar May 24 '24 20:05 czarcas7ic