[OTE-553] Skip sequence increment for timestamp nonce #1956
Conversation
defined protos defined keeper defined genesis
WalkthroughThe recent modifications enhance the Cosmos SDK's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnteHandler
participant AccountKeeper
User->>AnteHandler: Submit Transaction
AnteHandler->>AccountKeeper: Validate Transaction
AccountKeeper->>AnteHandler: Return Validation Result
alt Valid Transaction
AnteHandler->>AccountKeeper: Increment Sequence (if not Timestamp Nonce)
AccountKeeper->>AnteHandler: Update Account State
end
AnteHandler->>User: Return Transaction Result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
b5d93df to
7b47bc9
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/accountplus/keeper/keeper_test.go (1)
Line range hint
85-115:
Resolve merge conflict artifacts.The merge conflict artifacts (
<<<<<<< HEAD,=======,>>>>>>> timestamp-nonce-2) need to be removed to ensure the code compiles correctly.<<<<<<< HEAD expectedAccount := keeper.DefaultAccountState(constants.CarlAccAddress) ======= expectedAccount := types.AccountState{ Address: constants.CarlAccAddress.String(), TimestampNonceDetails: keeper.DeepCopyTimestampNonceDetails(keeper.InitialTimestampNonceDetails), } >>>>>>> timestamp-nonce-2 err := k.InitializeAccount(ctx, constants.CarlAccAddress) require.Nil(t, err, "Should be able to initialize account if it did not exist") actualAccount, found := k.GetAccountState(ctx, constants.CarlAccAddress) require.True(t, found, "Could not find account") require.Equal(t, actualAccount, expectedAccount)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/timestampnonce.go (1 hunks)
- protocol/app/ante/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (1 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (3 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Additional comments not posted (11)
protocol/x/accountplus/keeper/timestampnonce_test.go (1)
17-30: LGTM!The new test function
TestDeepCopyTimestampNonceDetailseffectively validates the deep copy functionality ofTimestampNonceDetails. It ensures that the original and copied instances are distinct and covers the necessary aspects of the deep copy operation.protocol/x/accountplus/keeper/timestampnonce.go (4)
11-11: LGTM!The constant
TimestampNonceSequenceCutoffis well-defined and sets a clear threshold for timestamp nonce sequences.
13-16: LGTM!The variable
InitialTimestampNonceDetailsis well-initialized and provides a clear starting point for timestamp nonce details.
18-32: LGTM!The function
DeepCopyTimestampNonceDetailsis well-implemented and ensures that a deep copy of theTimestampNonceDetailsis created. It handles nil input appropriately and copies the slice elements effectively.
34-36: LGTM!The function
IsTimestampNonceis well-implemented and provides a clear check for timestamp nonce sequences.protocol/app/ante/timestampnonce.go (3)
18-22: LGTM!The function correctly initializes the
DydxIncrementSequenceDecoratorstruct.
24-55: LGTM!The
AnteHandlefunction correctly implements the ante processing logic for the decorator.
14-16: LGTM!The
DydxIncrementSequenceDecoratorstruct and its methods are correctly defined.protocol/app/ante/timestampnonce_test.go (1)
19-74: LGTM!The
TestDydxIncrementSequenceDecoratorfunction correctly implements the test logic for the decorator.protocol/app/ante.go (2)
108-108: Ensure custom decorator implementation is correct.The custom decorator
customante.NewDydxIncrementSequenceDecoratorreplaces the standardante.NewIncrementSequenceDecorator. Ensure that the custom decorator implementation aligns with the expected behavior of the transaction sequence incrementing.
143-143: Ensure custom decorator implementation is correct.The custom decorator
customante.DydxIncrementSequenceDecoratorreplaces the standardante.IncrementSequenceDecorator. Ensure that the custom decorator implementation aligns with the expected behavior of the transaction sequence incrementing.
Comments failed to post (6)
protocol/x/accountplus/keeper/keeper.go
78-80: Acknowledged: Planned refactor for
InitializeAccount.The TODO comment suggests a refactor to improve efficiency by directly writing with timestamp-nonce.
Do you want me to assist with this refactor or open a GitHub issue to track this task?
protocol/x/accountplus/genesis.go
17-28: Reintroduce error handling for robustness.
The removal of error handling in
ExportGenesisassumes thatk.GetAllAccountStates(ctx)will not return an error. This assumption can lead to potential runtime issues if an error occurs. Reintroduce appropriate error handling to ensure robustness.- return &types.GenesisState{ - Accounts: k.GetAllAccountStates(ctx), + accounts, err := k.GetAllAccountStates(ctx) + if err != nil { + panic(err) + } + return &types.GenesisState{ + Accounts: accounts,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.accounts, err := k.GetAllAccountStates(ctx) if err != nil { panic(err) } return &types.GenesisState{ Accounts: accounts,protocol/x/accountplus/genesis_test.go
43-52: Resolve merge conflict artifacts.
The merge conflict artifacts (
<<<<<<< HEAD,=======,>>>>>>> timestamp-nonce-2) need to be removed to ensure the code compiles correctly.<<<<<<< HEAD ======= TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, }, { Address: constants.CarlAccAddress.String(), TimestampNonceDetails: types.TimestampNonceDetails{ >>>>>>> timestamp-nonce-2 TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, },Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, }, { Address: constants.CarlAccAddress.String(), TimestampNonceDetails: types.TimestampNonceDetails{ TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, },protocol/x/accountplus/keeper/keeper_test.go
13-19: Resolve merge conflict artifacts.
The merge conflict artifacts (
<<<<<<< HEAD,=======,>>>>>>> timestamp-nonce-2) need to be removed to ensure the code compiles correctly.<<<<<<< HEAD ======= "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/testutils" >>>>>>> timestamp-nonce-2Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."github.com/dydxprotocol/v4-chain/protocol/x/accountplus/testutils"
69-77: Resolve merge conflict artifacts.
The merge conflict artifacts (
<<<<<<< HEAD,=======,>>>>>>> timestamp-nonce-2) need to be removed to ensure the code compiles correctly.<<<<<<< HEAD err := k.InitializeAccount(ctx, constants.AliceAccAddress) ======= _, err := k.InitializeAccount(ctx, constants.AliceAccAddress) >>>>>>> timestamp-nonce-2 require.NotNil(t, err, "Account should not be able to be initialized if already exists")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements._, err := k.InitializeAccount(ctx, constants.AliceAccAddress) require.NotNil(t, err, "Account should not be able to be initialized if already exists")
26-58: Resolve merge conflict artifacts.
The merge conflict artifacts (
<<<<<<< HEAD,=======,>>>>>>> timestamp-nonce-2) need to be removed to ensure the code compiles correctly.<<<<<<< HEAD Accounts: []types.AccountState{ { Address: constants.AliceAccAddress.String(), TimestampNonceDetails: types.TimestampNonceDetails{ ======= Accounts: []*types.AccountState{ { Address: constants.AliceAccAddress.String(), TimestampNonceDetails: &types.TimestampNonceDetails{ >>>>>>> timestamp-nonce-2 TimestampNonces: []uint64{baseTsNonce + 1, baseTsNonce + 2, baseTsNonce + 3}, MaxEjectedNonce: baseTsNonce, }, }, { Address: constants.BobAccAddress.String(), <<<<<<< HEAD TimestampNonceDetails: types.TimestampNonceDetails{ ======= TimestampNonceDetails: &types.TimestampNonceDetails{ >>>>>>> timestamp-nonce-2 TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, }, },Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Accounts: []*types.AccountState{ { Address: constants.AliceAccAddress.String(), TimestampNonceDetails: &types.TimestampNonceDetails{ TimestampNonces: []uint64{baseTsNonce + 1, baseTsNonce + 2, baseTsNonce + 3}, MaxEjectedNonce: baseTsNonce, }, }, { Address: constants.BobAccAddress.String(), TimestampNonceDetails: &types.TimestampNonceDetails{ TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7}, MaxEjectedNonce: baseTsNonce + 1, }, }, },
7b47bc9 to
19da03c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/timestampnonce.go (1 hunks)
- protocol/app/ante/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/ante.go
- protocol/app/ante/timestampnonce.go
- protocol/app/ante/timestampnonce_test.go
- protocol/x/accountplus/keeper/timestampnonce.go
Additional comments not posted (1)
protocol/x/accountplus/keeper/timestampnonce_test.go (1)
Line range hint
1-7:
Imports look good.The imported packages are necessary and correctly used in the test function.
19da03c to
9e4e31b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/timestampnonce.go (1 hunks)
- protocol/app/ante/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- protocol/app/ante.go
- protocol/app/ante/timestampnonce.go
- protocol/app/ante/timestampnonce_test.go
- protocol/x/accountplus/keeper/timestampnonce.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
| ak: ak, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a comment saying this is forked from x/auth/ante/sigverify.go
There was a problem hiding this comment.
Will add in upcoming PR
| return sdk.Context{}, err | ||
| } | ||
|
|
||
| for _, signature := range signatures { |
There was a problem hiding this comment.
Note this technically changes the semantics of IncrementSequenceDecorator. It used to increment sequence for all sigTx.GetSigners() (everyone who's supposed to sign the transaction), now it's iterating over all the signatures (everyone who signed the transactions). These are not inherently the same set, so it's relying on upstream logic to call SigVerifyDecorator correctly (checking they are the same set).
There was a problem hiding this comment.
Is there a need to revert to iterating on signers and check that the signer has a signature?
| ak ante.AccountKeeper | ||
| } | ||
|
|
||
| func NewIncrementSequenceDecorator(ak ante.AccountKeeper) IncrementSequenceDecorator { |
There was a problem hiding this comment.
What's the rationale behind forking this decorator here, vs updating it in the dydxprotocol/cosmo-sdk fork? So that we can call accountpluskeeper helper function?
There was a problem hiding this comment.
I was under the impression that updating the fork required additional release complexity.
There is actually a ticket to deprecate IncrementSequenceDecorator altogether by adding the incrementation into sigverify. (https://linear.app/dydx/issue/OTE-624/combine-incrementsequencedecorator-into-sigverificationdecorator)
I think I will have time to address this ticket and the other comments before v6 release. Will publish new PR soon
| } | ||
|
|
||
| for _, signature := range signatures { | ||
| if accountpluskeeper.IsTimestampNonce(signature.Sequence) { |
There was a problem hiding this comment.
We have a very specific use case for timestamp nonce feature, and it seems to me the following will never happen:
- timestamp nonce transaction that requires multiple signers
- (vacuously true from above) timestamp nonce being used in combination with regular sequence
Given this and previous comment, the easiest/least disruptive change (and avoiding a fork) seems to be:
if !isShortTerm && !useTimestampNonce {
if ctx, err = h.incrementSequence.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
}
There was a problem hiding this comment.
I believe this will be addressed by the ticket i linked above (https://linear.app/dydx/issue/OTE-624/combine-incrementsequencedecorator-into-sigverificationdecorator)
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests