[OTE-553] Timestamp nonce #1970
Conversation
defined protos defined keeper defined genesis
WalkthroughThe recent updates enhance transaction handling and validation within the Changes
Sequence Diagram(s)sequenceDiagram
participant A as User
participant T as Transaction Handler
participant V as SigVerificationDecorator
participant K as Keeper
A->>T: Submit transaction
T->>T: Check nonce validity
T->>V: Validate signature
V->>K: Process timestamp nonce
K-->>T: Return nonce status
T-->>A: Confirm transaction status
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
protocol/x/accountplus/keeper/timestampnonce.go (3)
Line range hint
44-57:
Optimize the loop for better performance.The loop can be optimized by preallocating the slice for
newTsNoncesto avoid multiple allocations during append operations.func EjectStaleTimestampNonces(accountState *types.AccountState, referenceTs uint64) { tsNonceDetails := &accountState.TimestampNonceDetails newTsNonces := make([]uint64, 0, len(tsNonceDetails.TimestampNonces)) for _, tsNonce := range tsNonceDetails.TimestampNonces { if tsNonce >= referenceTs-MaxTimeInPastMs { newTsNonces = append(newTsNonces, tsNonce) } else { if tsNonce > tsNonceDetails.MaxEjectedNonce { tsNonceDetails.MaxEjectedNonce = tsNonce } } } tsNonceDetails.TimestampNonces = newTsNonces }
Line range hint
59-78:
Consider logging nonce rejection for better debugging.When a nonce is rejected, consider adding a log entry to help with debugging and monitoring.
if tsNonce <= tsNonceDetails.MaxEjectedNonce { fmt.Printf("Nonce %d rejected: less than or equal to MaxEjectedNonce %d", tsNonce, tsNonceDetails.MaxEjectedNonce) return false } if len(tsNonceDetails.TimestampNonces) < MaxTimestampNonceArrSize { tsNonceDetails.TimestampNonces = append(tsNonceDetails.TimestampNonces, tsNonce) return true } isSufficientlyLargeTsNonce, minIdx := isLargerThanSmallestValue(tsNonce, tsNonceDetails.TimestampNonces) if isSufficientlyLargeTsNonce { tsNonceDetails.MaxEjectedNonce = tsNonceDetails.TimestampNonces[minIdx] tsNonceDetails.TimestampNonces[minIdx] = tsNonce return true } fmt.Printf("Nonce %d rejected: not larger than smallest value", tsNonce) return false
Line range hint
80-99:
Consider handling edge cases for empty slices.The function currently handles empty slices by returning
false, -1, but consider adding a log entry for better debugging.// Check if input value is larger than smallest value in arr and return index of the min value. If minimum value has // duplicates, will return smallest index. index = -1 empty slice. func isLargerThanSmallestValue(value uint64, values []uint64) (bool, int) { if len(values) == 0 { fmt.Printf("Empty slice provided for comparison with value %d", value) return false, -1 } minIndex := 0 for i, ts := range values { if ts < values[minIndex] { minIndex = i } } if value > values[minIndex] { return true, minIndex } else { return false, minIndex } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- protocol/app/ante.go (5 hunks)
- protocol/app/ante/sigverify.go (4 hunks)
- protocol/lib/metrics/constants.go (2 hunks)
- protocol/x/accountplus/ante/timestampnonce.go (1 hunks)
- protocol/x/accountplus/ante/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (2 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (2 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/x/accountplus/keeper/keeper_test.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
Additional comments not posted (13)
protocol/x/accountplus/ante/timestampnonce.go (1)
1-9: LGTM! Imports are well-organized.The imports are correctly organized and relevant to the functionality provided in this file.
protocol/x/accountplus/keeper/keeper.go (1)
67-74: Potential risk of overwriting existing account states.The new method
GetAccountPlusStateWithTimestampNonceDetailsassumes that the caller ensures the account state does not already exist. This could lead to unintentional overwrites of account states.Ensure that the method is called only when the account state does not already exist. Consider adding a check or documentation to highlight this assumption.
protocol/x/accountplus/ante/timestampnonce_test.go (3)
1-17: LGTM! Imports and package declaration are well-organized.The imports and package declaration are correctly organized and relevant to the test functionality provided in this file.
19-73: Comprehensive test coverage.The tests cover various scenarios for the function
IsTimestampNonceTx, ensuring robust validation.
75-85: LGTM! Helper function is well-implemented.The helper function
getSignatureis correctly implemented and aids in creating test signatures.protocol/x/accountplus/keeper/timestampnonce.go (1)
23-42: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
ProcessTimestampNoncematch the new logic.Verification successful
Function usage and testing confirmed
The function
ProcessTimestampNonceis correctly used in the codebase and is adequately covered by test cases insigverify_test.go.
- Verified usage in
protocol/app/ante/sigverify.go.- Verified test coverage in
protocol/app/ante/sigverify_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessTimestampNonce` match the new logic. # Test: Search for the function usage. Expect: Only occurrences of the new logic. rg --type go -A 5 $'ProcessTimestampNonce'Length of output: 1802
Script:
#!/bin/bash # Extract relevant lines around the function call in sigverify.go rg --type go -A 10 -B 10 'ProcessTimestampNonce' protocol/app/ante/sigverify.go # Extract relevant lines around the test cases in sigverify_test.go rg --type go -A 10 -B 10 'ProcessTimestampNonce' protocol/app/ante/sigverify_test.goLength of output: 983
Script:
#!/bin/bash # Search for references to sigverify in the test files to ensure adequate testing of ProcessTimestampNonce rg --type go -A 10 -B 10 'sigverify' protocol/app/ante/sigverify_test.goLength of output: 823
Script:
#!/bin/bash # Search for test functions in sigverify_test.go to confirm execution of test cases involving ProcessTimestampNonce rg --type go -A 5 -B 5 'func Test' protocol/app/ante/sigverify_test.goLength of output: 643
protocol/app/ante/sigverify.go (2)
Line range hint
20-27:
LGTM!The initialization logic is clear and follows best practices.
103-120: Ensure telemetry metrics are consistent.The telemetry metrics for valid and invalid timestamp nonces should be consistent and provide meaningful insights.
protocol/app/ante.go (3)
156-156: Ensure the standard sequence increment logic is sufficient.The switch to the standard
ante.IncrementSequenceDecoratorshould be verified to ensure it meets all requirements previously handled by the custom implementation.
117-117: Ensure the standard sequence increment logic is sufficient.The switch to the standard
ante.NewIncrementSequenceDecoratorshould be verified to ensure it meets all requirements previously handled by the custom implementation.
117-117: Ensure the standard sequence increment logic is sufficient.The switch to the standard
ante.NewIncrementSequenceDecoratorshould be verified to ensure it meets all requirements previously handled by the custom implementation.protocol/lib/metrics/constants.go (2)
14-14: LGTM!The addition of the
ExecModeconstant is correctly placed and consistent with the existing constants.
412-412: LGTM!The addition of the
TimestampNonceconstant is correctly placed and consistent with the existing constants.
| // IsTimestampNonceTx returns `true` if the supplied `tx` consist of a single signature that uses a timestamp nonce | ||
| // value for sequence | ||
| func IsTimestampNonceTx(ctx sdk.Context, tx sdk.Tx) (bool, error) { | ||
| sigTx, ok := tx.(authsigning.SigVerifiableTx) | ||
| if !ok { | ||
| return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") | ||
| } | ||
| signatures, err := sigTx.GetSignaturesV2() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if len(signatures) != 1 { | ||
| return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature") | ||
| } | ||
|
|
||
| if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) { | ||
| return true, nil | ||
| } else { | ||
| return false, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure comprehensive error handling.
The function IsTimestampNonceTx correctly checks the transaction type and retrieves signatures. However, consider logging the errors for better traceability.
- return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
+ err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
+ ctx.Logger().Error(err.Error())
+ return false, err
- return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
+ err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
+ ctx.Logger().Error(err.Error())
+ return false, errCommittable 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.
| // IsTimestampNonceTx returns `true` if the supplied `tx` consist of a single signature that uses a timestamp nonce | |
| // value for sequence | |
| func IsTimestampNonceTx(ctx sdk.Context, tx sdk.Tx) (bool, error) { | |
| sigTx, ok := tx.(authsigning.SigVerifiableTx) | |
| if !ok { | |
| return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") | |
| } | |
| signatures, err := sigTx.GetSignaturesV2() | |
| if err != nil { | |
| return false, err | |
| } | |
| if len(signatures) != 1 { | |
| return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature") | |
| } | |
| if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) { | |
| return true, nil | |
| } else { | |
| return false, nil | |
| } | |
| } | |
| // IsTimestampNonceTx returns `true` if the supplied `tx` consist of a single signature that uses a timestamp nonce | |
| // value for sequence | |
| func IsTimestampNonceTx(ctx sdk.Context, tx sdk.Tx) (bool, error) { | |
| sigTx, ok := tx.(authsigning.SigVerifiableTx) | |
| if !ok { | |
| err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") | |
| ctx.Logger().Error(err.Error()) | |
| return false, err | |
| } | |
| signatures, err := sigTx.GetSignaturesV2() | |
| if err != nil { | |
| return false, err | |
| } | |
| if len(signatures) != 1 { | |
| err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature") | |
| ctx.Logger().Error(err.Error()) | |
| return false, err | |
| } | |
| if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) { | |
| return true, nil | |
| } else { | |
| return false, nil | |
| } | |
| } |
| accountState, found := k.GetAccountState(ctx, address) | ||
| if !found { | ||
| // initialize accountplus state with ts nonce details | ||
| k.SetAccountState(ctx, address, GetAccountPlusStateWithTimestampNonceDetails(address, tsNonce)) | ||
| } else { | ||
| EjectStaleTimestampNonces(&accountState, blockTs) | ||
| tsNonceAccepted := AttemptTimestampNonceUpdate(tsNonce, &accountState) | ||
| if !tsNonceAccepted { | ||
| return fmt.Errorf("timestamp nonce %d rejected", tsNonce) | ||
| } | ||
| k.SetAccountState(ctx, address, accountState) | ||
| } |
There was a problem hiding this comment.
Consider logging account state initialization.
When initializing the account state, consider adding a log entry to help with debugging and monitoring.
if !found {
// initialize accountplus state with ts nonce details
k.SetAccountState(ctx, address, GetAccountPlusStateWithTimestampNonceDetails(address, tsNonce))
fmt.Printf("Initialized account state for address: %s with tsNonce: %d", address, tsNonce)
} else {
EjectStaleTimestampNonces(&accountState, blockTs)
tsNonceAccepted := AttemptTimestampNonceUpdate(tsNonce, &accountState)
if !tsNonceAccepted {
return fmt.Errorf("timestamp nonce %d rejected", tsNonce)
}
k.SetAccountState(ctx, address, accountState)
}| func (k Keeper) ProcessTimestampNonce(ctx sdk.Context, acc sdk.AccountI, tsNonce uint64) error { | ||
| blockTs := uint64(ctx.BlockTime().UnixMilli()) | ||
| address := acc.GetAddress() | ||
|
|
||
| if !IsValidTimestampNonce(tsNonce, blockTs) { | ||
| return fmt.Errorf("timestamp nonce %d not within valid time window", tsNonce) | ||
| } |
There was a problem hiding this comment.
Ensure the error message is user-friendly.
The error message "timestamp nonce %d not within valid time window" could be more user-friendly by providing additional context or guidance on acceptable time windows.
- return fmt.Errorf("timestamp nonce %d not within valid time window", tsNonce)
+ return fmt.Errorf("timestamp nonce %d not within the valid time window. Ensure the nonce is within %d ms of the current block time.", tsNonce, MaxTimeInPastMs)Committable suggestion was skipped due to low confidence.
protocol/app/ante/sigverify.go
Outdated
| start := time.Now() | ||
| err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence) | ||
|
|
||
| if !accountpluskeeper.IsValidTimestampNonce(tsNonce, blockTs) { | ||
| return ctx, errorsmod.Wrapf( | ||
| sdkerrors.ErrWrongSequence, | ||
| "timestamp nonce %d not within valid time window", tsNonce, | ||
| telemetry.MeasureSince(start, []string{metrics.TimestampNonce, metrics.Latency}...) | ||
| if err == nil { | ||
| telemetry.IncrCounterWithLabels( | ||
| []string{metrics.TimestampNonce, metrics.Valid, metrics.Count}, | ||
| 1, | ||
| []gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))}, | ||
| ) | ||
| } | ||
| accountState, found := svd.akp.GetAccountState(ctx, address) | ||
| if !found { | ||
| err := svd.akp.InitializeAccountWithTimestampNonceDetails(ctx, address, tsNonce) | ||
| if err != nil { | ||
| return ctx, errorsmod.Wrapf( | ||
| sdkerrors.ErrLogic, | ||
| fmt.Sprintf("failed to initialize AccountState for address %d", address), | ||
| ) | ||
| } | ||
| return ctx, nil | ||
| } else { | ||
| accountpluskeeper.EjectStaleTimestampNonces(&accountState, blockTs) | ||
| tsNonceAccepted := accountpluskeeper.AttemptTimestampNonceUpdate(tsNonce, &accountState) | ||
| if !tsNonceAccepted { | ||
| return ctx, errorsmod.Wrapf( | ||
| sdkerrors.ErrWrongSequence, | ||
| "timestamp nonce %d rejected", tsNonce, | ||
| ) | ||
| } | ||
| svd.akp.SetAccountState(ctx, address, accountState) | ||
| telemetry.IncrCounterWithLabels( | ||
| []string{metrics.TimestampNonce, metrics.Invalid, metrics.Count}, | ||
| 1, | ||
| []gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))}, | ||
| ) | ||
| return ctx, errorsmod.Wrapf(sdkerrors.ErrWrongSequence, err.Error()) |
There was a problem hiding this comment.
Consider logging nonce processing duration for better monitoring.
The duration of nonce processing can be logged for better monitoring and performance analysis.
start := time.Now()
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)
duration := time.Since(start)
telemetry.MeasureSince(start, []string{metrics.TimestampNonce, metrics.Latency}...)
if err == nil {
telemetry.IncrCounterWithLabels(
[]string{metrics.TimestampNonce, metrics.Valid, metrics.Count},
1,
[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
)
fmt.Printf("Processed valid timestamp nonce %d in %s", sig.Sequence, duration)
return ctx, nil
} else {
telemetry.IncrCounterWithLabels(
[]string{metrics.TimestampNonce, metrics.Invalid, metrics.Count},
1,
[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
)
fmt.Printf("Failed to process timestamp nonce %d in %s: %s", sig.Sequence, duration, err.Error())
return ctx, errorsmod.Wrapf(sdkerrors.ErrWrongSequence, err.Error())
}| var isTimestampNonce bool | ||
| if isTimestampNonce, err = accountplusante.IsTimestampNonceTx(ctx, tx); err != nil { | ||
| return ctx, err | ||
| } | ||
|
|
||
| if !isShortTerm && !isTimestampNonce { |
There was a problem hiding this comment.
Consider logging the timestamp nonce check for better debugging.
The check for timestamp nonce transactions can be logged for better debugging and monitoring.
var isTimestampNonce bool
if isTimestampNonce, err = accountplusante.IsTimestampNonceTx(ctx, tx); err != nil {
return ctx, err
}
fmt.Printf("Transaction %s is a timestamp nonce: %t", tx.GetTxHash(), isTimestampNonce)
if !isShortTerm && !isTimestampNonce {
if ctx, err = h.incrementSequence.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
}|
Will fix typo flagged by golangcli-lint after remainder of tests finish running |
0e28134 to
5625b2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/accountplus/keeper/timestampnonce.go (1)
31-34: Consider logging account state initialization.When initializing the account state, consider adding a log entry to help with debugging and monitoring.
if !found { // initialize accountplus state with ts nonce details k.SetAccountState(ctx, address, GetAccountPlusStateWithTimestampNonceDetails(address, tsNonce)) + fmt.Printf("Initialized account state for address: %s with tsNonce: %d", address, tsNonce) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- protocol/app/ante.go (5 hunks)
- protocol/app/ante/sigverify.go (4 hunks)
- protocol/lib/metrics/constants.go (2 hunks)
- protocol/x/accountplus/ante/timestampnonce.go (1 hunks)
- protocol/x/accountplus/ante/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (2 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (2 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- protocol/app/ante.go
- protocol/app/ante/sigverify.go
- protocol/lib/metrics/constants.go
- protocol/x/accountplus/ante/timestampnonce.go
- protocol/x/accountplus/ante/timestampnonce_test.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
| require.Equal(t, actualAccount, expectedAccount) | ||
| }) | ||
| } | ||
| // TODO: add explicit unit tests for Get and Set funcs |
There was a problem hiding this comment.
Reduction in test coverage.
The removal of the TestInitializeAccount function reduces the current test coverage. Consider adding the new unit tests for Get and Set functions as soon as possible to maintain test coverage.
Do you want me to help create the new unit tests or open a GitHub issue to track this task?
| func GetAccountPlusStateWithTimestampNonceDetails( | ||
| address sdk.AccAddress, | ||
| tsNonce uint64, | ||
| ) error { | ||
| if _, found := k.GetAccountState(ctx, address); found { | ||
| msg := "Cannot initialize AccountState for address with existing AccountState, address: " + address.String() | ||
| k.Logger(ctx).Error(msg) | ||
| return errors.New(msg) | ||
| } | ||
|
|
||
| accountState := types.AccountState{ | ||
| ) types.AccountState { | ||
| return types.AccountState{ |
There was a problem hiding this comment.
Reintroduce error handling and logging.
The new method GetAccountPlusStateWithTimestampNonceDetails removes error handling and logging, which could lead to potential issues if the method is called with an address that already has an associated account state. Consider adding error handling and logging to ensure data integrity and ease of debugging.
func GetAccountPlusStateWithTimestampNonceDetails(
address sdk.AccAddress,
tsNonce uint64,
) types.AccountState {
+ // Add error handling and logging
+ if address == nil {
+ return types.AccountState{}, fmt.Errorf("address cannot be nil")
+ }
+ log.Printf("Generating account state for address: %s with tsNonce: %d", address.String(), tsNonce)
return types.AccountState{
Address: address.String(),
TimestampNonceDetails: types.TimestampNonceDetails{
MaxEjectedNonce: TimestampNonceSequenceCutoff,
TimestampNonces: []uint64{tsNonce},
},
}
}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.
| func GetAccountPlusStateWithTimestampNonceDetails( | |
| address sdk.AccAddress, | |
| tsNonce uint64, | |
| ) error { | |
| if _, found := k.GetAccountState(ctx, address); found { | |
| msg := "Cannot initialize AccountState for address with existing AccountState, address: " + address.String() | |
| k.Logger(ctx).Error(msg) | |
| return errors.New(msg) | |
| } | |
| accountState := types.AccountState{ | |
| ) types.AccountState { | |
| return types.AccountState{ | |
| func GetAccountPlusStateWithTimestampNonceDetails( | |
| address sdk.AccAddress, | |
| tsNonce uint64, | |
| ) types.AccountState { | |
| // Add error handling and logging | |
| if address == nil { | |
| return types.AccountState{}, fmt.Errorf("address cannot be nil") | |
| } | |
| log.Printf("Generating account state for address: %s with tsNonce: %d", address.String(), tsNonce) | |
| return types.AccountState{ | |
| Address: address.String(), | |
| TimestampNonceDetails: types.TimestampNonceDetails{ | |
| MaxEjectedNonce: TimestampNonceSequenceCutoff, | |
| TimestampNonces: []uint64{tsNonce}, | |
| }, | |
| } | |
| } |
| if !IsValidTimestampNonce(tsNonce, blockTs) { | ||
| return fmt.Errorf("timestamp nonce %d not within valid time window", tsNonce) | ||
| } |
There was a problem hiding this comment.
Enhance error message for timestamp nonce validation.
The error message "timestamp nonce %d not within valid time window" could be more user-friendly by providing additional context or guidance on acceptable time windows.
return fmt.Errorf("timestamp nonce %d not within valid time window", tsNonce)
+ return fmt.Errorf("timestamp nonce %d not within the valid time window. Ensure the nonce is within %d ms of the current block time.", tsNonce, MaxTimeInPastMs)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.
| if !IsValidTimestampNonce(tsNonce, blockTs) { | |
| return fmt.Errorf("timestamp nonce %d not within valid time window", tsNonce) | |
| } | |
| if !IsValidTimestampNonce(tsNonce, blockTs) { | |
| return fmt.Errorf("timestamp nonce %d not within the valid time window. Ensure the nonce is within %d ms of the current block time.", tsNonce, MaxTimeInPastMs) | |
| } |
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 (4 hunks)
- protocol/app/ante/sigverify.go (5 hunks)
- protocol/x/accountplus/ante/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/keeper_test.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/sigverify.go
- protocol/x/accountplus/ante/timestampnonce.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
| // TODO: add explicit unit tests for Get and Set funcs | ||
| // https://linear.app/dydx/issue/OTE-633/add-explicit-unit-tests-for-get-and-set-methods-for-accountplus-keeper |
There was a problem hiding this comment.
Reminder: Add explicit unit tests for Get and Set functions.
The TODO comment indicates the need to add unit tests for Get and Set functions. The removal of the TestInitializeAccount function has reduced the current test coverage.
Do you want me to help create the new unit tests or open a GitHub issue to track this task?
protocol/app/ante/sigverify.go
Outdated
| @@ -100,10 +99,14 @@ func (svd SigVerificationDecorator) AnteHandle( | |||
| // `GoodTilBlock` for replay protection. | |||
| if !skipSequenceValidation { | |||
| if accountpluskeeper.IsTimestampNonce(sig.Sequence) { | |||
| start := time.Now() | |||
| defer telemetry.ModuleMeasureSince( | |||
There was a problem hiding this comment.
This needs to be put within ProcessTimestampNonce so that the defer clause is run when ProcessTimestampNonce returns. This is a common pattern to measure the latency of a function from start to finish
There was a problem hiding this comment.
defer telemetry.ModuleMeasureSince(
accountplustypes.ModuleName,
time.Now(),
metrics.TimestampNonce,
metrics.Latency,
)
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)
A bit confused. As u said defer run after the function below is run. In this case it is right above
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)
So it will run after ProcessTimestampNonce?
There was a problem hiding this comment.
A defer statement defers the execution of a function until the surrounding function returns. https://go.dev/tour/flowcontrol/12
| @@ -1,3 +1,4 @@ | |||
| package keeper_test | |||
|
|
|||
| // TODO: add explicit unit tests for Get and Set funcs | |||
| // https://linear.app/dydx/issue/OTE-633/add-explicit-unit-tests-for-get-and-set-methods-for-accountplus-keeper | |||
There was a problem hiding this comment.
Styling convention:
// TODO(OTE-634): add explicit unit tests for get and set methods for accountplus keeper
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/app/ante/sigverify.go (3 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/ante/sigverify.go
- protocol/x/accountplus/keeper/timestampnonce.go
Changelist
Refactor
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores