feat: sdk v0.50.x upgrade #8274

czarcas7ic · 2024-05-17T00:01:38Z

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:

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 dydxprotocol/v4-chain#867
[CORE-538] Integrate AutoCLI as part of Cosmos 0.47 -> 0.50 upgrade and re-add a few standalone commands that changed packages dydxprotocol/v4-chain#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.
Benchmark how much the new SQS block processing logic slows down sync speed.

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 · 2024-05-21T01:28:40Z

go.mod


- // Using branch osmo/v0.37.4
+ // UNFORKING v2 TODO: No longer use wasmd fork, added logic to app.toml override to use 1000 instead of default 100 cache size, which was the reason for having the fork in the first place.


czarcas7ic · 2024-05-21T01:28:49Z

go.mod

- // github.com/osmosis-labs/osmosis/osmoutils => ./osmoutils
- // github.com/osmosis-labs/osmosis/x/epochs => ./x/epochs
- // github.com/osmosis-labs/osmosis/x/ibc-hooks => ./x/ibc-hooks
+ // UNFORKING v2 TOOD: Need to manually define these until we tag, it keeps trying to "upgrade" which actually downgrades us to the old sdk sub module logic


p0mvn · 2024-05-21T01:56:24Z

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 · 2024-05-21T01:59:05Z

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

Great work on powering through this!

I'm far from being done but just wanted to leave some initial comments as to avoid a single large review cycles

p0mvn · 2024-05-21T02:09:06Z

app/app.go

+ app.SetStreamingManager(
+ storetypes.StreamingManager{
+ ABCIListeners: []storetypes.ABCIListener{sqsStreamingService},
+ StopNodeOnErr: true,


Note: to self: would like to look into this more pre-merge

p0mvn · 2024-05-21T02:15:35Z

x/valset-pref/validator_set.go

+ delegation, err := k.stakingKeeper.GetDelegation(ctx, delegator, val.ValAddr)
+ if err != nil {
 return fmt.Errorf("No delegation found for delegator %s to validator %s\n", delegator, val.ValAddr)
 }


Is this logically equivalent to before?

From my understanding, the error can be returned in 2 cases:

database error

the key is actually not present.

Should we also format errors so that the clients can know why there is no delegation?

Alternatively, maybe we could bubble up the error without any custom message.

An ditto for most other calls of GetDelegation

Yeah agreed with this, for simplicity I decided to just bubble up the error, because the sdk returns an error now instead of false if the value is nil (https://github.com/osmosis-labs/cosmos-sdk/blob/1a5662f2a4586735a64388386eaf23a62dfddad9/x/staking/keeper/delegation.go#L30-L32)

Previously we still returned the delegation even if the value was nil (https://github.com/cosmos/cosmos-sdk/blob/7009a2e0cdb93b286199bbaa22e8880aeec83c8c/x/staking/keeper/delegation.go#L15-L27)

Change here c780c73

p0mvn · 2024-05-21T02:22:25Z

x/txfees/module.go

@@ -175,48 +190,42 @@ func (AppModule) ConsensusVersion() uint64 { return 1 }
 // If they have, we unmarshal the current consensus params, update the target gas, and cache the value.
 // This is done to improve performance by not having to fetch and unmarshal the consensus params on every block.
 // TODO: Move this to EIP-1559 code
+// UNFORKING v2 TODO: Do we still want to use cachedConsParams here? I guess it removes the need to do arithmetic operations on every block.


Makes sense for me to keep

p0mvn · 2024-05-21T13:15:48Z

cmd/osmosisd/cmd/root.go

- initRootCmd(rootCmd, encodingConfig)
+ initRootCmd(rootCmd, encodingConfig, tempApp)
+
+ // UNFORKING v2 TODO: I don't think we have an option but to implement this. With out, the sdk queries do not show up in the CLI.


Why would we not want to implement this? Is it because we already have osmocli that is customized and tailored to our needs?

czarcas7ic · 2024-05-21T20:41:09Z

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:

osmosis/x/tokenfactory/keeper/before_send.go

Lines 121 to 122 in 7c213f8

 From: from.String(), 

 To: to.String(),

p0mvn

In this cycle, reviewed:

pool manager
protorev
smart-account
superfluid
tokenfactory
twap
txfees
valset-pref

There were some tests that I will aim to come back to reviewing in more detail after 1 full iteration.

p0mvn · 2024-05-21T15:28:05Z

ante/sendblock.go

@@ -70,9 +73,14 @@ func (decorator *SendBlockDecorator) CheckIfBlocked(msgs []sdk.Msg) error {
 return nil
 }
 for _, msg := range msgs {
- signers := msg.GetSigners()
+ // UNFORKING v2 TODO: GetSigners is no longer available


Could we start tracking to do some specific testnet tests on this?

p0mvn · 2024-05-21T15:35:40Z

x/superfluid/keeper/stake.go

+ if err != nil {
+ panic(err)
+ }


I think that we should generally break APIs and bubble up any errors.

For example, this panic might lead to a chain halt because we do not have a panic-catching hook for begin block:

osmosis/x/superfluid/keeper/epoch.go

Line 60 in 62141e5

k.RefreshIntermediaryDelegationAmounts(ctx, accs)

Yes, good call, doing now

p0mvn · 2024-05-21T15:37:36Z

x/superfluid/keeper/slash_test.go

- []int64{0},
- []int64{0},
+ []int64{},
+ []int64{}, // UNFORKING v2 TODO: We no longer can slash unbonded validators, verify that this is correct


Could you share more context on this?

Why did this change?

p0mvn · 2024-05-21T15:46:17Z

x/superfluid/keeper/intermediary_account.go

@@ -91,7 +100,10 @@ func (k Keeper) GetOrCreateIntermediaryAccount(ctx sdk.Context, denom, valAddr s
 // create a new account. We use base accounts, as this is what's done for cosmwasm smart contract accounts.
 // and in the off-chance someone manages to find a bug that forces the account's creation.
 if !k.ak.HasAccount(ctx, intermediaryAcct.GetAccAddress()) {
- k.ak.SetAccount(ctx, authtypes.NewBaseAccount(intermediaryAcct.GetAccAddress(), nil, 0, 0))
+ // UNFORKING v2 TODO: I now need to set NextAccountNumber with k.ak.NextAccountNumber(ctx) instead of using zero, due to new invariant checks.


I'm not quite sure - why do we now need this and why it wasn't needed before. Could you please link the invariants mentioned?

p0mvn · 2024-05-21T15:50:14Z

x/superfluid/keeper/edge_case_test.go

@@ -142,3 +146,85 @@ func (s *KeeperTestSuite) TestTryUnbondingSuperfluidLockupDirectly() {
 })
 }
 }
+
+func (s *KeeperTestSuite) handleEquivocationEvidence(ctx context.Context, evidence *types.Equivocation) error {


Was this purely copied from the old method that was removed?

Yes exactly, this is now a private method in the sdk, but its kind of silly given all the methods within it are public. So instead of straying further from the sdk, and because this is only used in tests, I added here manually.

p0mvn · 2024-05-21T15:56:02Z

x/smart-account/authenticator/authentication_request.go

@@ -68,7 +71,15 @@ func GetSignerAndSignatures(tx sdk.Tx) (signers []sdk.AccAddress, signatures []s
 }

 // Retrieve messages from the transaction.
- signers = sigTx.GetSigners()
+ // UNFORKING v2 TODO: I dont know if ranging over the address bytes and assigning to AccAddress is correct


I am not 100% sure of the internal details but this looks reasonable to me. I think that we can rely on the unit and integration tests for asserting correctness

p0mvn · 2024-05-21T23:38:51Z

osmoutils/cosmwasm/helpers_test.go

-
- return rawCWAddr, bech32CWAddr
-}
+// TESTS MOVED DIRECTLY TO x/cosmwasmpool/pool_module_test.go to prevent circular imports (specifically on osmosis app for the test suite)


Makes sense - shall we delete then?

p0mvn · 2024-05-21T23:41:34Z

x/smart-account/ante/ante_test.go

- // ensure only the right amount of sigs have been checked
- if tc.checks > 0 {
- s.Require().GreaterOrEqual(res.GasMeter().GasConsumed(), uint64(baseGas+(tc.checks-1)*approachingGasPerSig))
- s.Require().LessOrEqual(res.GasMeter().GasConsumed(), uint64(baseGas+tc.checks*approachingGasPerSig))
- } else {
- if tc.checkGas {
- s.Require().LessOrEqual(res.GasMeter().GasConsumed(), uint64(baseGas))
- }
- }


Note to self: come back to this

I don't quite get why this is removed

Actually this was likely removed by @nicolaslara when he corrected the remaining issues in the smart accounts module. He will likely be able to expand further on why this was safe (good call pointing it out though)

I think we can look at this module by itself after the merge

p0mvn

lockup
mint
pool-incentives

only one comment

p0mvn · 2024-05-22T01:17:13Z

x/lockup/keeper/gas_test.go

@@ -75,6 +75,7 @@ func (s *KeeperTestSuite) TestRepeatedLockTokensDistinctDurationGas() {

 avgGas, maxGas := s.measureAvgAndMaxLockGas(totalNumLocks, defaultAddr, coinsFn, durFn)
 fmt.Printf("test deets: total locks created %d\n", totalNumLocks)
- s.Assert().LessOrEqual(int(avgGas), 100000, "average gas / lock")
+ // UNFORKING v2 TODO: This increased from 100000


I think that's fine

p0mvn

Reviewed all remaining modules. Skipped protos, tests, and simulation

Have remaining:

osmoutils
osmomath
SQS integration
app wiring

x/concentrated-liquidity/types/msgs_test.go

p0mvn · 2024-05-23T16:48:36Z

go.mod

- // v1.0.0-beta.3 is incompatible, so we use v1.0.0-beta.2
- github.com/cosmos/cosmos-proto => github.com/cosmos/cosmos-proto v1.0.0-beta.2
+ // Using branch osmo/v0.38.x
+ // https://github.com/osmosis-labs/cometbft/releases/tag/v0.37.4-v25-osmo-2


nit is this tag relevant?

It is not, will replace tags when getting closer to release time. I suppose I could also just make an rc0

p0mvn · 2024-05-23T16:54:55Z

ingest/sqs/service/sqs_streaming_service.go

+func (s *sqsStreamingService) processBlockChangeSet() error {
+ if s.changeSet == nil {
+ return nil
+ }
+
+ for _, kv := range s.changeSet {
+ for _, listener := range s.writeListeners[s.storeKeyMap[kv.StoreKey]] {
+ if err := listener.OnWrite(s.storeKeyMap[kv.StoreKey], kv.Key, kv.Value, kv.Delete); err != nil {
+ return err
+ }
+ }
+ }
+
+ return nil
+}


I wonder how this affects block time processing. We should create an issue and track validating that the performance hit is acceptable relative to the expected lowered block time

Added this to the list of things to test

p0mvn · 2024-05-23T16:55:44Z

osmomath/go.mod

-// github.com/osmosis-labs/osmosis/osmoutils => ../osmoutils
-)
+// Our cosmos-sdk branch is: https://github.com/osmosis-labs/cosmos-sdk/tree/osmo/v0.50.x, current branch: osmo/v0.50.x. Direct commit link: https://github.com/osmosis-labs/cosmos-sdk/commit/1a5662f2a4586735a64388386eaf23a62dfddad9
+// https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.47.5-v25-osmo-1


nit: is the tag correct?

It is not, am waiting closer to release to do the first tag, but can do it sooner if needed

PaddyMc

Great work here!

Review all module updates, protos, wiring and ran in-place-testnet also read through and validated the UNFORKING TODOs

Created this doc to help with the UNFORKING comments:
https://www.notion.so/osmosiszone/Unforking-TODOs-9c17730015164849afc81e99ee8d1146

This reverts commit 5c8698b.

czarcas7ic · 2024-05-24T20:15:38Z

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

Thanks all for the reviews!

p0mvn

I've looked over the remaining files and don't have more to point out other than what's already been discussed or noted in unforking TODOs.

Great work @czarcas7ic

czarcas7ic added 30 commits May 7, 2024 15:25

initial push

6d70096

delete

8536841

tidy

fdab2ba

add replace

9dddc1b

proto

b986140

excludes

9fcbd6b

update go mods

b28e6ff

fix

114125c

initial pass through modules

1595fd8

checkpoint

768f2b1

checkpoint

7995b58

another round

8bfa7f1

fork update with fixes

53f94a6

fork bumps

e9d96ce

more fixes

a3d52ff

move to cosmosdb

38cda38

signModeHandler fix

da8bd95

more fixes

ec9e0aa

store fixes

29bea81

checkpoint

c719bd3

checkpoint

2b0d471

more event fixes

cd3d9e2

GetSigners fix for tests

5f20366

get signers fixes

2fabe91

smart account fixes

0569420

fixes

9175a70

checkpoint

3bebd10

txfees completion

7214f3e

more errors

1bcb8be

rosetta import

196b215

czarcas7ic requested review from p0mvn and AlpinYukseloglu as code owners May 20, 2024 22:42

clean up go mods

1226a75

czarcas7ic commented May 21, 2024

View reviewed changes

Merge branch 'main' into adam/v0.50.x-upgrade

62141e5

p0mvn reviewed May 21, 2024

View reviewed changes

Merge branch 'main' into adam/v0.50.x-upgrade

7c213f8

fix errors for GetDelegation

c780c73

p0mvn reviewed May 21, 2024

View reviewed changes

p0mvn reviewed May 22, 2024

View reviewed changes

czarcas7ic and others added 3 commits May 21, 2024 20:44

return error instead of panic

b68ea8a

chore: fix build

04447de

Merge branch 'main' into adam/v0.50.x-upgrade

665c586

p0mvn reviewed May 23, 2024

View reviewed changes

x/concentrated-liquidity/types/msgs_test.go Outdated Show resolved Hide resolved

p0mvn reviewed May 23, 2024

View reviewed changes

czarcas7ic added 5 commits May 23, 2024 22:23

fix weird error check

117194a

Merge branch 'main' into adam/v0.50.x-upgrade

b1c4f68

ignore words

24964a4

more spelling fixes

5c8698b

final spelling errors

9b287c5

PaddyMc approved these changes May 24, 2024

View reviewed changes

czarcas7ic added 2 commits May 24, 2024 11:23

Revert "more spelling fixes"

eb3121f

This reverts commit 5c8698b.

redo spelling check

15895cd

czarcas7ic merged commit 5bff216 into main May 24, 2024
1 check passed

czarcas7ic deleted the adam/v0.50.x-upgrade branch May 24, 2024 20:16

p0mvn reviewed May 30, 2024

View reviewed changes


		// Using branch osmo/v0.37.4
		// UNFORKING v2 TODO: No longer use wasmd fork, added logic to app.toml override to use 1000 instead of default 100 cache size, which was the reason for having the fork in the first place.

feat: sdk v0.50.x upgrade #8274

feat: sdk v0.50.x upgrade #8274

Conversation

czarcas7ic commented May 17, 2024 • edited

What is the purpose of the change

References

Notes

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented May 21, 2024

p0mvn commented May 21, 2024 • edited

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic commented May 21, 2024 • edited

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaddyMc left a comment

Choose a reason for hiding this comment

czarcas7ic commented May 24, 2024 • edited

p0mvn left a comment

Choose a reason for hiding this comment

czarcas7ic commented May 17, 2024 •

edited

p0mvn commented May 21, 2024 •

edited

p0mvn May 21, 2024 •

edited

czarcas7ic May 21, 2024 •

edited

czarcas7ic commented May 21, 2024 •

edited

czarcas7ic May 22, 2024 •

edited

p0mvn left a comment •

edited

czarcas7ic commented May 24, 2024 •

edited