-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(client/tx): simulate with correct pk #18472
Conversation
### Walkthrough
The changes involve updates to the Cosmos SDK, focusing on transaction simulation enhancements, bug fixes, and test improvements. A new `fromName` field is added to the `Factory` struct to manage key information for transactions. The testing suite is updated by renaming packages and adding new tests for simulation public keys and signature data. The changelog summarizes bug fixes, including a data race resolution, transaction simulation accuracy, and consistency in viper prefix settings.
### Changes
| File(s) | Summary |
| --- | --- |
| `client/tx/factory.go` | Introduced `fromName` field in `Factory` struct, updated `NewFactoryCLI` and `getSimPK` for key management, added `multisig` import, and added `getSimSignatureData` method for simulation transactions. |
| `client/tx/factory_test.go`, `client/tx/aux_builder_test.go`, `client/tx/legacy_test.go`, `client/tx/tx_test.go` | Renamed package from `tx_test` to `tx`, updated references, and added new test functions for simulation keys and signature data. |
| `CHANGELOG.md` | Documented bug fixes and improvements related to transaction simulation, data race resolution, and viper prefix consistency. |
> 🌟 In the code's woven tapestry, a rabbit hops with glee,
> 🍂 As autumn leaves fall gently, bugs flee with a decree.
> 🛠️ With each commit and merge, the SDK grows more free,
> 🌐 A cosmos of code, evolving for you and me. TipsChat with CodeRabbit Bot (
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/tx/factory.go (3 hunks)
Additional comments: 2
client/tx/factory.go (2)
33-39: The addition of the
fromName
field to theFactory
struct is a significant change. Ensure that all usages of theFactory
struct throughout the codebase are updated to handle this new field appropriately. This may include serialization and deserialization logic if the struct is persisted or transmitted.93-99: The initialization of the
fromName
field in theNewFactoryCLI
function is correct. It uses theFromName
from theclientCtx
, which should be the intended behavior for setting up the factory with the correct key name for transaction simulation and signing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/tx/factory.go (3 hunks)
Additional comments: 3
client/tx/factory.go (3)
33-39: The addition of the
fromName
field to theFactory
struct is a significant change. Ensure that all usages of theFactory
struct throughout the codebase are updated to handle this new field appropriately. This includes initialization, serialization, and any other operations that might be performed onFactory
instances.93-99: The initialization of the
fromName
field in theNewFactoryCLI
function is correct. It uses theFromName
field from theclientCtx
which is expected to be populated with the name of the key to be used for the transaction. This is a critical change for the issue being addressed, as it ensures that the correct public key is used during transaction simulation.437-449: The modification to the
getSimPK
method to retrieve the public key using thefromName
field is the core change that addresses the issue. It is important to ensure that thekeybase
is notnil
and that thesimulateAndExecute
flag is true before attempting to retrieve the key. This prevents unnecessary keybase access when the user is only doing a dry run (--dry-run
flag) and not an actual transaction simulation with execution (--gas=auto
flag). The error handling and the default public key initialization look appropriate.
Thanks for the PR @JulianToledano! Is there a way that we can lock in a test for this behavior and to prevent regressions? Seems quite odd to me that we were using the wrong public key, but are also changing it though can't validate this change nor show which one fails. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this still requires use of the keyring, albeit it can use any key you provide. Ideally, we avoid the keyring all together? WDYT?
Have you tested with the steps in the original issue?
I think accessing to the keyring is needed at some point. We either access the keyring at factory construction time and load the pubkey as a field or create a Lines 27 to 32 in 5f36ad0
Yes, although the main issue is reported on #18476. Real problem is we are always simulating with a
|
Sure I can try to add one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- client/tx/factory.go (6 hunks)
Additional comments: 6
client/tx/factory.go (6)
15-21: The import of the
multisig
package is correctly placed in the import block, following Go conventions of grouping standard library, third-party, and own module imports separately. This change is necessary for the new functionality related to multisig keys.34-40: The addition of the
fromName
field to theFactory
struct is a significant change. It is important to ensure that all parts of the code that instantiate or manipulate theFactory
struct are aware of this new field and handle it appropriately. This change is necessary for the new functionality to specify the key name for transactions.94-100: The
NewFactoryCLI
function has been updated to initialize thefromName
field withclientCtx.FromName
. This is a critical change that ensures thefromName
field is set correctly when theFactory
is instantiated. It is important to ensure that theclientCtx.FromName
is being set correctly upstream.409-415: The
BuildSimTx
function has been updated to create aSignatureV2
with a public key and signature data obtained from the newgetSimSignatureData
method. This change is crucial for ensuring that the simulation uses the correct public key and signature data format based on the key type. It is important to ensure that theSetSignatures
method of the transaction builder can handle theSignatureV2
type correctly and that this change does not introduce any issues with transaction signing.436-448: The
getSimPK
method has been modified to retrieve the public key from the keybase using thefromName
field ifsimulateAndExecute
is true and the keybase is not nil. This is a critical change that ensures the correct public key is used for simulation. It is important to verify that the keybase properly handles theKey
method and that thefromName
field is being set with a valid key name.454-471: The new
getSimSignatureData
method determines the appropriate signature data type based on the public key type. This method handles both single and multisig public keys, creating aSingleSignatureData
orMultiSignatureData
as needed. This change is important for supporting different key types in transaction simulations. It is important to ensure that thesignMode
is set correctly for each signature data type and that the multisig threshold is respected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/tx/factory_test.go (1 hunks)
Additional comments: 3
client/tx/factory_test.go (3)
1-2: The package name change from
tx_test
totx
means that the tests are no longer in a separate package. This could potentially expose test files to production builds and might allow access to internal package functions or variables that should not be exposed. Verify that this change is intentional and consider the implications of making test files part of the production package.46-92: The new test function
TestFactory_getSimPK
is well-structured and seems to cover the necessary cases for testing the retrieval of simulated public keys. However, ensure that thewant
field in the test cases is properly initialized with expected values rather than empty structs to make the tests meaningful.94-117: Similar to the previous test function,
TestFactory_getSimSignatureData
should also have thewant
field in the test cases initialized with expected values for the tests to be effective.
Makes sense @JulianToledano -- let's proceed then 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- client/tx/aux_builder_test.go (3 hunks)
- client/tx/factory_test.go (1 hunks)
- client/tx/legacy_test.go (1 hunks)
- client/tx/tx_test.go (9 hunks)
Files skipped from review due to trivial changes (1)
- client/tx/legacy_test.go
Additional comments: 14
client/tx/factory_test.go (3)
1-19: The package name change from
tx_test
totx
and the removal of thetx.
prefix in import paths is a significant change. Ensure that this does not introduce any circular dependencies and that the tests still run as expected in the new package structure.46-92: The new test function
TestFactory_getSimPK
is well-structured and tests the functionality of thegetSimPK
method. It checks for both simple and multisig keys, which is good for coverage. Ensure that thegenKey
function correctly sets up the keys in the keyring for the test cases.94-117: The new test function
TestFactory_getSimSignatureData
is also well-structured and tests the functionality of thegetSimSignatureData
method. It checks for both single and multisig public keys, which is good for coverage. Ensure that thegetSimSignatureData
method returns the correct type ofsigning.SignatureData
for different public key types.client/tx/aux_builder_test.go (2)
1-10: The package name change from
tx_test
totx
and the removal of thetx
import are consistent with the summary provided. This change will allow the test file to access unexported functions and types within thetx
package, which is a common practice in Go for white-box testing.25-26: The addition of
counterModule.RegisterInterfaces(reg)
is necessary for the test case "GetAuxSignerData works for DIRECT_AUX" to ensure that the necessary interfaces are registered for the test to run successfully.client/tx/tx_test.go (9)
1-4: The package name change from
tx_test
totx
is significant as it changes the package from an external test package to the internal package. This allows the tests to access unexported functions, types, and constants within thetx
package. Ensure that this change is intentional and that the tests still cover the intended functionality from an external perspective.9-19: The removal of the
github.com/cosmos/cosmos-sdk/client/tx
import is appropriate since the functions and types from thetx
package are now being used directly without thetx.
prefix due to the package name change. This is a good cleanup.80-86: The
Factory
struct is now being used without thetx.
prefix, which is correct given the package name change. However, ensure that theFactory
struct and its methods are correctly exported from thetx
package to be used in this test file.89-95: The test case logic appears correct, and the renaming of
CalculateGas
function usage without thetx.
prefix is consistent with the package name change. However, ensure that theCalculateGas
function is correctly exported from thetx
package.106-110: The
mockTxFactory
function is correctly updated to useFactory
without thetx.
prefix. Ensure that theFactory
struct and its methods are correctly exported from thetx
package.195-201: The test case for
TestMnemonicInMemo
is correctly updated to useFactory
without thetx.
prefix. Ensure that theFactory
struct and its methods are correctly exported from thetx
package.266-272: The test cases in
TestSign
are correctly updated to useFactory
without thetx.
prefix. Ensure that theFactory
struct and its methods are correctly exported from thetx
package.353-359: The test logic for
TestSign
appears correct, and the renaming ofSign
function usage without thetx.
prefix is consistent with the package name change. However, ensure that theSign
function is correctly exported from thetx
package.408-414: The test case for
TestPreprocessHook
is correctly updated to useFactory
without thetx.
prefix. Ensure that theFactory
struct and its methods are correctly exported from thetx
package. Additionally, verify that thePreprocessTx
function is correctly exported and that thePreprocessTxFn
type is correctly handled within thetx
package.
for _, tc := range testcases { | ||
tc := tc | ||
t.Run(tc.name, func(t *testing.T) { | ||
b = tx.NewAuxTxBuilder() | ||
b = NewAuxTxBuilder() | ||
err := tc.malleate() | ||
|
||
if tc.expErr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of t.Run
to run subtests is a good practice as it allows for running tests in parallel and provides better test output. However, the assignment b = NewAuxTxBuilder()
inside the loop without using the :=
syntax will overwrite the b
variable in the outer scope for each test case, which could lead to data races if tests are run in parallel. It's important to ensure that each test case is independent and does not affect the others.
To ensure that each test case has its own instance of AuxTxBuilder
, you can modify the code as follows:
- b = NewAuxTxBuilder()
+ b := NewAuxTxBuilder()
This change will scope the b
variable to each subtest, preventing data races and ensuring that each test case is isolated.
Commitable 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.
for _, tc := range testcases { | |
tc := tc | |
t.Run(tc.name, func(t *testing.T) { | |
b = tx.NewAuxTxBuilder() | |
b = NewAuxTxBuilder() | |
err := tc.malleate() | |
if tc.expErr { | |
for _, tc := range testcases { | |
tc := tc | |
t.Run(tc.name, func(t *testing.T) { | |
b := NewAuxTxBuilder() | |
err := tc.malleate() | |
if tc.expErr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
@@ -200,7 +199,7 @@ func TestAuxTxBuilder(t *testing.T) { | |||
for _, tc := range testcases { | |||
tc := tc | |||
t.Run(tc.name, func(t *testing.T) { | |||
b = tx.NewAuxTxBuilder() | |||
b = NewAuxTxBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a reason to change the package name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's for testing their private function getSimSignatureData
and set fromName
on the Factory that is usually private. For the latter, they could use clientCtx with a from name however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 69-69: The changelog entries are clear and provide direct links to the pull requests for detailed reference. It's good practice to include such links as it helps in tracking the changes and for anyone looking into the history of changes. The descriptions are concise and explain the nature of the fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work and thank you @JulianToledano! Just one more round then LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- client/tx/factory.go (6 hunks)
- client/tx/factory_test.go (1 hunks)
Additional comments: 10
client/tx/factory_test.go (4)
1-19: The package and import renaming from
tx_test
totx
and the associated import path updates are standard refactoring practices to maintain consistency after such changes. Ensure that the renaming does not conflict with other packages and that all references to the package within the codebase are updated accordingly.21-44: The
TestFactoryPrepare
function tests thePrepare
method of theFactory
struct. The tests seem to cover different scenarios, including offline context and setting specific account numbers and sequences. Ensure that theMockAccountRetriever
is correctly mocking the expected behavior and that the test cases cover all relevant scenarios for thePrepare
method.46-93: The
TestFactory_getSimPKType
function tests thegetSimPK
method of theFactory
struct. The test cases cover different types of keys, including simple and multisig keys. It's important to ensure that thegenKey
function within each test case is generating the keys correctly and that thewantType
accurately reflects the expected type of public key. The use of table-driven tests is a good practice for clarity and maintainability.95-119: The
TestFactory_getSimSignatureData
function tests thegetSimSignatureData
method of theFactory
struct. The test cases are designed to check if the correct signature data type is returned for different types of public keys. Similar to the previous test function, ensure that the test cases are comprehensive and cover all possible public key types that thegetSimSignatureData
method is expected to handle.client/tx/factory.go (6)
15-21: The addition of new imports, especially the
multisig
package, should be verified to ensure they are used within the code and there are no unused imports left which can lead to unnecessary bloat.34-40: The addition of the
fromName
field to theFactory
struct is a key change. Ensure that all references to theFactory
struct throughout the codebase are updated to handle this new field if necessary. This change should be documented in the changelog to inform users of the update.94-100: The initialization of the
fromName
field within theNewFactoryCLI
function is correct and aligns with the description provided in the pull request summary. This ensures that thefromName
is set when theFactory
is created, which is necessary for the new functionality to work as intended.409-415: The
BuildSimTx
function has been updated to include a newSignatureV2
structure with the public key obtained fromgetSimPK
and signature data fromgetSimSignatureData
. This is a crucial change for the simulation to work with the correct public key. Ensure that theSequence
is correctly set as it is used in the transaction signing process.436-448: The
getSimPK
method retrieves the public key from the keybase using thefromName
. This is a significant change that addresses the issue mentioned in the pull request summary. Ensure that thesimulateAndExecute
flag is correctly interpreted and that the key retrieval logic is robust and handles possible errors gracefully.454-470: The new
getSimSignatureData
method is used to determine the correctSignatureData
type based on the public key type. This is particularly important for multisig scenarios. Ensure that the logic correctly handles different types of public keys and that the multisig threshold is respected when creating theMultiSignatureData
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/tx/factory.go (6 hunks)
Additional comments: 6
client/tx/factory.go (6)
15-21: The addition of the
multisig
package import is necessary for the new functionality related to handling multisig public keys. This change is appropriate given the context of the pull request.34-40: The addition of the
fromName
field to theFactory
struct is a key part of the update, allowing the factory to store the name of the key to be used for transactions. This is a necessary change for the bug fix and is implemented correctly.94-100: The
NewFactoryCLI
function has been updated to initialize thefromName
field withclientCtx.FromName
. This change is consistent with the pull request's goal to ensure the correct public key is used during transaction simulation.409-415: The
BuildSimTx
function has been modified to usef.getSimSignatureData(pk)
for setting signature data. This is a crucial change that allows the function to handle different types of public keys, especially in multisig scenarios. The implementation appears to be correct and aligns with the pull request's objectives.436-448: The
getSimPK
function has been modified to retrieve the public key from the keybase using thefromName
. This change is essential for ensuring that the correct public key is used during simulation. The error handling and retrieval logic are correctly implemented.454-471: The new
getSimSignatureData
method is added to determine the correct signature data type for simulation transactions. This method handles the case for multisig public keys by creating aMultiSignatureData
instance with the appropriate number ofSingleSignatureData
placeholders. The logic here is sound and matches the requirements for multisig transaction simulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thank you @JulianToledano!
(cherry picked from commit 80e0c63) # Conflicts: # client/tx/tx_test.go
(cherry picked from commit 80e0c63) # Conflicts: # CHANGELOG.md # client/tx/aux_builder_test.go # client/tx/factory_test.go # client/tx/tx_test.go
Description
Closes:
#17711
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers 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...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Bug Fixes
BaseApp.getContext
.Refactor
Tests
Documentation