refactor(auth): group sig verification ante handlers (SigGasConsume, SigVerify, IncreaseSequence) #18817
Conversation
WalkthroughThe recent update streamlines the authentication process in the x/auth module by combining multiple decorators into a single Changes
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 X ? TipsChat with CodeRabbit Bot (
|
WalkthroughThe recent changes revolve around optimizing gas consumption and signature verification in a blockchain application. The Changes
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 X ? TipsChat with CodeRabbit Bot (
|
testinginprod
changed the title
CHANGELOG.md
Outdated
| @@ -193,6 +193,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| * (x/upgrade) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) Upgrade module no longer stores the app version but gets and sets the app version stored in the `ParamStore` of baseapp. | |||
| * (x/staking) [#17655](https://github.com/cosmos/cosmos-sdk/pull/17655) `HistoricalInfo` was replaced with `HistoricalRecord`, it removes the validator set and comet header and only keep what is needed for IBC. | |||
| * (x/auth) [#18817](https://github.com/cosmos/cosmos-sdk/pull/18817) SigVerification, GasConsumption, IncreaseSequence ante decorators have all been joined into one SigVerification decorator. Gas consumption during TX validation flow has reduced. | |||
There was a problem hiding this comment.
nit, changelog placement (x/auth/changelog).
can we have an upgrading.md section as well (## Unreleased > ### Simapp > ##### xxx)
|
Could you update the description of the pr before merging. Helps when we look back for any reason. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | ||
| ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), | ||
| ante.NewIncrementSequenceDecorator(options.AccountKeeper), | ||
| ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer), |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The verification process has confirmed that:
- The
SigGasConsumerfield is present in theante.HandlerOptionsand is initialized at least insimapp/app.gowithante.DefaultSigVerificationGasConsumer. - References to
SigGasConsumerare found in various files, including test files such asante_test.goandtestutil_test.go, which suggests that its behavior is covered by unit tests.
Based on the information gathered, it appears that the SigGasConsumer is properly initialized and tested. However, without explicit confirmation that all instances of HandlerOptions are correctly initializing SigGasConsumer, there remains a possibility that some initializations could be missing.
- Verify that all instances of
ante.HandlerOptionsacross the codebase properly initialize theSigGasConsumerfield. - Confirm that the unit tests cover all scenarios involving
SigGasConsumer, especially in the context of the newNewSigVerificationDecoratorfunctionality.
Analysis chain
The addition of options.SigGasConsumer to NewSigVerificationDecorator is consistent with the refactor to consolidate signature verification and gas consumption. Ensure that the SigGasConsumer is properly initialized before being passed to this function and that its behavior is covered by unit tests.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that SigGasConsumer is initialized wherever HandlerOptions is constructed
rg --type go 'HandlerOptions{'
# Check for the presence of unit tests involving SigGasConsumer
rg --type go 'Test.*SigGasConsumer' ./x/auth/ante
Length of output: 178
Script:
#!/bin/bash
# Correct the regex pattern to escape the curly brace and verify that SigGasConsumer is initialized wherever HandlerOptions is constructed
rg --type go 'HandlerOptions\{'
# Use a more general pattern to check for the presence of unit tests involving SigGasConsumer
rg --type go 'SigGasConsumer'
Length of output: 2625
| ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), | ||
| ante.NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators | ||
| ante.NewValidateSigCountDecorator(options.AccountKeeper), | ||
| ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | ||
| ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), | ||
| ante.NewIncrementSequenceDecorator(options.AccountKeeper), | ||
| ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer), | ||
| } |
There was a problem hiding this comment.
The error handling for required components (AccountKeeper, BankKeeper, SignModeHandler) is in place. Confirm that similar checks are performed for SigGasConsumer to ensure it is not nil, as it is now a critical part of the NewSigVerificationDecorator.
…ip/auth/ante_refactor_2
Description
In this PR we are grouping the certain auth ante decorators, specifically:
Have been merged with SigVerification decorator.
This brings:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...