fix: align signer extraction adapter for mempool remove #19759
Conversation
this change should have been included in 5452586
WalkthroughWalkthroughThe recent update enhances 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 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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- types/mempool/priority_nonce.go (3 hunks)
- types/mempool/priority_nonce_test.go (1 hunks)
Additional comments: 2
types/mempool/priority_nonce.go (1)
- 431-437: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [434-443]
The updated logic in the
Removefunction to usemp.cfg.SignerExtractor.GetSigners(tx)for extracting signers from transactions is a significant improvement. This change aligns with the PR's objective to refine the signer extraction process. However, it's crucial to ensure that theSignerExtractorinterface and itsGetSignersmethod are robust and handle errors gracefully. Additionally, considering the removal of thecosmossdk.io/x/auth/signingimport, verify that no other parts of the file or related modules depend on it for functionality that hasn't been replaced or refactored.types/mempool/priority_nonce_test.go (1)
- 85-142: The test
TestPriorityNonceTxOrderWithAdaptereffectively demonstrates the functionality of thesignerExtractionAdapterin a priority nonce mempool scenario. A few observations and suggestions:
- The test is well-structured and covers the scenario of switching between the old and new signer extraction methods. This is crucial for ensuring backward compatibility and the correctness of the new implementation.
- The use of
require.NoErrorfor asserting the absence of errors is appropriate and keeps the test code clean.- The dynamic generation of test cases using
simtypes.RandomAccountsand the iteration over these cases provide a good level of coverage.This test is a valuable addition to the test suite, ensuring the new signer extraction logic works as expected in various scenarios.
| @@ -54,6 +54,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (types) [#18768](https://github.com/cosmos/cosmos-sdk/pull/18768) Add MustValAddressFromBech32 function. | |||
| * (gRPC) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) Add debug log prints for each gRPC request. | |||
| * (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module. | |||
| * (types) [#19759](https://github.com/cosmos/cosmos-sdk/pull/19759) Align SignerExtractionAdapter in PriorityNonceMempool Remove. | |||
There was a problem hiding this comment.
Please specify the version or release section for this changelog entry to ensure it's correctly categorized. This helps in maintaining the clarity and organization of the changelog, making it easier for users to understand the changes in each release.
types/mempool/priority_nonce_test.go
Outdated
| type signerExtractionAdapter struct { | ||
| UseOld bool | ||
| } | ||
|
|
||
| func (a signerExtractionAdapter) GetSigners(tx sdk.Tx) ([]mempool.SignerData, error) { | ||
| if a.UseOld { | ||
| sigs, err := tx.(signing.SigVerifiableTx).GetSignaturesV2() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| signerData := make([]mempool.SignerData, len(sigs)) | ||
| for _, sig := range sigs { | ||
| signerData = append(signerData, mempool.SignerData{ | ||
| Signer: sig.PubKey.Address().Bytes(), | ||
| Sequence: sig.Sequence, | ||
| }) | ||
| } | ||
| return signerData, nil | ||
| } | ||
| signerData, err := mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx) | ||
| return signerData, err | ||
| } |
There was a problem hiding this comment.
The implementation of signerExtractionAdapter and its method GetSigners introduces a flexible way to switch between signer extraction methods based on the UseOld flag. However, there are a few points to consider:
- The use of
signing.SigVerifiableTxdirectly in theGetSignersmethod assumes that all transactions passed to this method will implement this interface. While this is likely the case, it's a good practice to check the type assertion's success to avoid potential runtime panics. - The method appends to
signerDatawithout initializing it with a specific length, which can be less efficient than allocating the exact required size upfront. - The fallback to
mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx)whenUseOldis false is a good approach, but it creates a new instance ofDefaultSignerExtractionAdapterevery time. Consider if there's a more efficient way to handle this, perhaps by reusing an instance.
Consider checking the type assertion's success and initializing slices with the exact required size for efficiency. Also, evaluate the possibility of reusing the DefaultSignerExtractionAdapter instance to avoid unnecessary allocations.
yihuang
added
the
backport/v0.50.x
types/mempool/priority_nonce_test.go
Outdated
| } | ||
|
|
||
| func (a signerExtractionAdapter) GetSigners(tx sdk.Tx) ([]mempool.SignerData, error) { | ||
| if a.UseOld { |
There was a problem hiding this comment.
can we revert the if, to make code more readable
if !a.UseOld {
return mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx)
}
... more boilerplate code
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/mempool/priority_nonce_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- types/mempool/priority_nonce_test.go
(cherry picked from commit 26c1db6) # Conflicts: # CHANGELOG.md
) (#19773) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com>
Description
Closes: #XXXX
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...
Summary by CodeRabbit