refactor(types,client)!: refactor global address sdk.Config #18372
Conversation
julienrbrt
changed the title
bizk
changed the title
![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: 10
Configuration used: CodeRabbit UI
Files selected for processing (14)
- client/context.go (2 hunks)
- client/keys/add.go (1 hunks)
- client/keys/add_ledger_test.go (4 hunks)
- client/keys/add_test.go (2 hunks)
- client/keys/delete_test.go (1 hunks)
- client/keys/export_test.go (1 hunks)
- client/keys/rename_test.go (1 hunks)
- crypto/ledger/ledger_mock.go (1 hunks)
- tests/integration/runtime/query_test.go (1 hunks)
- testutil/key.go (2 hunks)
- testutil/key_test.go (2 hunks)
- types/address.go (1 hunks)
- types/config.go (3 hunks)
- types/config_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/integration/runtime/query_test.go
Additional comments: 13
crypto/ledger/ledger_mock.go (1)
- 42-47:
The change fromsdk.GetConfig().GetCoinType()tosdk.GetAddressConfig().GetCoinType()correctly reflects the new configuration pattern.client/keys/export_test.go (1)
- 93-95: The change from
sdk.GetConfig().GetFullBIP44Path()tosdk.GetAddressConfig().GetFullBIP44Path()aligns with the refactor to use the newAddressConfigstruct. Ensure that theGetAddressConfig()method is properly implemented and that it returns a validAddressConfiginstance with the correct BIP44 path configuration.client/keys/add.go (1)
- 82-82: The
flagCoinTypeis now correctly using theGetAddressConfig().GetCoinType()method to set the default coin type for HD derivation. This change aligns with the refactor to use the newAddressConfigstruct instead of the globalsdk.Config. Ensure that all other parts of the codebase that rely on the coin type are updated to useGetAddressConfig()as well.client/context.go (2)
75-79: The
AddressConfigsfield has been added to theContextstruct to hold the newsdk.AddressConfig. This change aligns with the pull request's goal to refactor address configurations.338-342: The
WithAddressConfigmethod has been correctly implemented to allow updating theContextwith a specificsdk.AddressConfig. This method is essential for the new modular approach to address configuration.client/keys/delete_test.go (1)
- 36-36:
The update to usesdk.GetAddressConfig().GetFullBIP44Path()aligns with the refactor to use the newAddressConfigstruct.client/keys/rename_test.go (1)
- 33-33:
The update to usesdk.GetAddressConfig().GetFullBIP44Path()aligns with the refactor of the globalsdk.Configto use the newAddressConfigstruct.testutil/key_test.go (2)
18-24:
The refactoring fromGetConfig()toGetAddressConfig()is correctly implemented here.43-49:
The changes are consistent with the new address configuration pattern.testutil/key.go (2)
14-20: The
NewMnemonicfunction is being called withsdk.NewAddressConfig().GetFullBIP44Path(), which creates a newAddressConfigeach time the function is called. This could lead to inconsistencies if theAddressConfigis meant to be a singleton or if its instantiation is expensive. Verify ifAddressConfigshould be instantiated once and reused.63-71: Similar to the previous comment,
sdk.NewAddressConfig().GetFullBIP44Path()is used multiple times within the same function, potentially creating multiple instances ofAddressConfig. Consider reusing a single instance if applicable.client/keys/add_ledger_test.go (2)
32-41: The
configobject is being used to set Bech32 prefixes, but theaddressConfigobject is used to set the BIP-44 purpose and coin type. This is inconsistent with the summary which states that the globalsdk.Configis being refactored in favor ofAddressConfig. Verify that the Bech32 prefix settings are also meant to be migrated to the newAddressConfigstructure or if they should remain part of the globalsdk.Config.94-100: The
addressConfigis being set again with the same values as before, and the globalconfigis being used to set Bech32 prefixes again. This seems redundant and could be a mistake. Verify if this repetition is intentional or if it should be removed to avoid confusion and potential errors.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (12)
- client/keys/add.go (1 hunks)
- client/keys/add_ledger_test.go (4 hunks)
- client/keys/add_test.go (1 hunks)
- client/keys/delete_test.go (1 hunks)
- crypto/ledger/ledger_mock.go (1 hunks)
- crypto/ledger/ledger_test.go (1 hunks)
- simapp/simd/cmd/testnet.go (1 hunks)
- testutil/key.go (3 hunks)
- testutil/key_test.go (4 hunks)
- testutil/network/network.go (3 hunks)
- types/address.go (2 hunks)
- types/config_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- client/keys/add.go
Additional comments: 15
client/keys/add_ledger_test.go (1)
- 39-40: Verify if the
addressConfigshould also be set with a custom coin type to match the test's intention of using a custom configuration.client/keys/add_test.go (1)
- 221-238: The changes to the address configuration system are correctly implemented in the test. The
addressConfigis properly initialized and used in theclientCtx, and thepathvariable is correctly derived fromclientCtx.AddressConfigs.GetFullBIP44Path()and used in thekb.NewAccountcall.crypto/ledger/ledger_mock.go (1)
- 42-47: The removal of the derivation path check against
sdk.GetConfig().GetCoinType()is consistent with the refactor to use the newAddressConfigstruct. Ensure that the validation logic is now correctly implemented elsewhere in the codebase to maintain security and correctness.crypto/ledger/ledger_test.go (1)
- 19-21: The
TestPublicKeyUnsafefunction appears to be unchanged, but ensure that the changes in the global address configuration do not affect the expected behavior of this test, especially since thesdk.CoinTypemight now be coming from a different configuration source.simapp/simd/cmd/testnet.go (1)
- 276-279: The addition of
sdk.NewAddressConfig().GetFullBIP44Path()as an argument toGenerateSaveCoinKeyaligns with the new address configuration system. Ensure thatGenerateSaveCoinKeyis updated to handle this new argument and that all other parts of the codebase that interact with address configurations are consistently using the newAddressConfigstruct and methods.testutil/key.go (3)
14-20: The update to use
sdk.NewAddressConfig().GetFullBIP44Path()inGenerateCoinKeyis consistent with the refactor of the address configuration system.37-43: The addition of the
hdPathparameter toGenerateSaveCoinKeyfunction signature is consistent with the refactor and allows for more flexible key generation.64-72: The logic for generating or recovering a new account using the provided mnemonic and
hdPathis correctly implemented and aligns with the new address configuration system.testutil/key_test.go (4)
21-21: The summary states that
NewFundraiserParamshas been replaced withtypes.CoinType, but the code still usesNewFundraiserParams. Please verify if the summary is accurate or if the code needs to be updated.35-35: The update to use
types.NewAddressConfig().GetFullBIP44Path()inTestGenerateSaveCoinKeyaligns with the changes described in the summary.61-61: The update to use
types.NewAddressConfig().GetFullBIP44Path()inTestGenerateSaveCoinKeyOverwriteFlagaligns with the changes described in the summary.35-35: Ensure that all parts of the codebase that require the BIP44 path have been updated to use the new
types.NewAddressConfig().GetFullBIP44Path()method.Also applies to: 61-61
testutil/network/network.go (3)
135-135: The addition of
AddressConfigto theConfigstruct aligns with the refactor to centralize address configuration.167-167: Initialization of
AddressConfigin theDefaultConfigfunction is consistent with the refactor to use the new address configuration system.461-461: The update to include
cfg.AddressConfig.GetFullBIP44Path()as an argument totestutil.GenerateSaveCoinKeyis consistent with the changes to the address configuration system.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- types/address.go (2 hunks)
- types/config_test.go (1 hunks)
Additional comments: 1
types/config_test.go (1)
- 20-22: The
TestKeyringServiceNametest case remains valid and unaffected by the refactor.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/keys/delete_test.go (1 hunks)
Additional comments: 2
client/keys/delete_test.go (2)
36-36: The update to use
sdk.GetAddressConfig().GetFullBIP44Path()for thepathvariable is consistent with the refactor of the global address configuration within the SDK.37-37: Skip this comment as it has been previously addressed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- client/keys/add_ledger_test.go (4 hunks)
- types/address.go (2 hunks)
Additional comments: 2
types/address.go (2)
731-735: The
NewAddressConfigfunction is correctly named and initializes theAddressConfigwith default values.738-740: The
GetFullBIP44Pathmethod correctly returns the BIP44 path using thepurposeandcoinTypefields.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (10)
- client/keys/add_ledger_test.go (3 hunks)
- client/keys/add_test.go (1 hunks)
- client/keys/delete_test.go (1 hunks)
- client/keys/export_test.go (1 hunks)
- client/keys/rename_test.go (1 hunks)
- simapp/simd/cmd/testnet.go (1 hunks)
- testutil/key.go (3 hunks)
- testutil/key_test.go (4 hunks)
- testutil/network/network.go (1 hunks)
- types/address.go (2 hunks)
Files skipped from review due to trivial changes (2)
- client/keys/export_test.go
- testutil/key_test.go
Additional comments: 13
client/keys/add_ledger_test.go (3)
35-37: The code correctly sets the Bech32 prefixes for account, validator, and consensus node addresses. This is necessary for the test to reflect the address formats used in the specific blockchain network being tested.
52-56: The test case
Test_runAddCmdLedgerWithCustomCoinTypeis correctly setting up the command and its flags. It's important to ensure that the flags are consistent with the command's expected behavior.86-87: The test case
Test_runAddCmdLedgerWithCustomCoinTypechecks the string representation of the public key. It's important to ensure that the expected public key string is correct and matches the output of the ledger device or mock being used.client/keys/add_test.go (1)
- 233-235: The change from
sdk.GetConfig().GetFullBIP44Path()tosdk.GetFullBIP44Path()aligns with the refactor in the SDK to centralize the management of address configurations. Ensure that the new methodsdk.GetFullBIP44Path()is correctly implemented and that it provides the expected BIP44 path in the context of this test.client/keys/delete_test.go (1)
- 36-36: The change in how the BIP44 path is retrieved is consistent with the refactor described in the pull request summary. Ensure that the new method
sdk.GetFullBIP44Path()is correctly implemented and that all tests and usages have been updated accordingly.client/keys/rename_test.go (1)
- 33-33: The change from
sdk.GetConfig().GetFullBIP44Path()tosdk.GetFullBIP44Path()should be verified to ensure that the new function provides the correct BIP44 path without requiring additional context or configuration.simapp/simd/cmd/testnet.go (1)
- 276-276: The use of
sdk.GetFullBIP44Path()to provide the HD path for theGenerateSaveCoinKeyfunction aligns with the newAddressConfigstruct and its methods. Ensure that thesdk.GetFullBIP44Path()function is implemented correctly and returns the expected BIP44 path based on the new address configuration system.testutil/key.go (3)
14-20: The update to use
sdk.GetFullBIP44Path()directly inGenerateCoinKeyis consistent with the refactor described in the summary.37-41: The addition of the
hdPathparameter toGenerateSaveCoinKeyaligns with the refactor to improve key management by introducing more structured handling of BIP-0044 paths.64-72: The use of the
hdPathparameter in theGenerateSaveCoinKeyfunction for generating or recovering a new account is in line with the changes described in the summary.testutil/network/network.go (1)
- 458-458: The addition of
sdk.GetFullBIP44Path()as an argument totestutil.GenerateSaveCoinKeyis consistent with the changes described in the summary. Ensure that thesdk.GetFullBIP44Path()function is implemented as expected and that all necessary parts of the codebase are updated to use this new approach for obtaining the BIP44 path.types/address.go (2)
32-35: The removal of commented-out configuration setup lines is a good cleanup, considering the new
AddressConfigsystem.713-714: The new
GetFullBIP44Pathfunction correctly uses thePurposeandCoinTypeconstants to return the BIP44 path.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/keys/delete_test.go (1 hunks)
Additional comments: 1
client/keys/delete_test.go (1)
- 36-36: The change in how the BIP44 path is retrieved aligns with the refactor described in the pull request summary. Ensure that the new method
sdk.GetFullBIP44Path()is correctly implemented and that it does not introduce any regressions or unexpected behavior in the test cases.
WalkthroughWalkthroughThe overarching change involves the removal of global configuration for address coin types and purposes, as well as the global Changes
Assessment against linked issues
The assessment indicates that while the main objectives related to the removal of global TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (17)
- CHANGELOG.md (1 hunks)
- client/keys/add.go (1 hunks)
- client/keys/add_ledger_test.go (3 hunks)
- client/keys/add_test.go (1 hunks)
- client/keys/delete_test.go (1 hunks)
- client/keys/export_test.go (1 hunks)
- client/keys/rename_test.go (1 hunks)
- crypto/ledger/ledger_mock.go (1 hunks)
- crypto/ledger/ledger_test.go (1 hunks)
- simapp/simd/cmd/testnet.go (1 hunks)
- tests/integration/runtime/query_test.go (1 hunks)
- testutil/key.go (3 hunks)
- testutil/key_test.go (4 hunks)
- testutil/network/network.go (1 hunks)
- types/address.go (2 hunks)
- types/config.go (3 hunks)
- types/config_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- client/keys/rename_test.go
- tests/integration/runtime/query_test.go
Additional comments: 20
CHANGELOG.md (1)
- 69-69: The changelog entry correctly summarizes the significant change made by pull request #18372. It is clear and concise, indicating the removal of the global configuration for address coin type and purpose.
client/keys/add.go (1)
- 79-85: The change from
sdk.GetConfig().GetCoinType()tosdk.CoinTypeinflagCoinTypeis consistent with the refactor to remove global configuration. Ensure thatsdk.CoinTypeis properly initialized before this point in the code.client/keys/add_ledger_test.go (2)
32-41: The removal of global Bech32 prefix configuration is consistent with the refactor to remove global configurations. Ensure that the Bech32 prefix is now being set correctly elsewhere in the codebase.
49-56: The addition of
cmd := AddKeyCommand()andcmd.Flags().AddFlagSet(Commands().PersistentFlags())in the first test function is consistent with the refactor to remove global configurations and to ensure that the command is properly configured for the test.client/keys/add_test.go (1)
- 233-235: The change from
sdk.GetConfig().GetFullBIP44Path()tosdk.GetFullBIP44Path()aligns with the refactor to remove global configuration and use direct function calls instead. Ensure that the new functionsdk.GetFullBIP44Path()provides the correct BIP44 path as expected in the context of this test.client/keys/delete_test.go (1)
- 36-36: The update to use
sdk.GetFullBIP44Path()directly aligns with the refactor to remove global configuration for address coin type and purpose. Ensure that the new method provides the correct BIP44 path as expected in the context of this test.client/keys/export_test.go (1)
- 93-95: The change from
sdk.GetConfig().GetFullBIP44Path()tosdk.GetFullBIP44Path()is consistent with the refactor described in the pull request summary. Ensure that the new method provides the correct BIP44 path as expected by theNewAccountfunction.crypto/ledger/ledger_mock.go (1)
- 42-43: The removal of the derivation path validation against the coin type is a significant change. Ensure that the new validation logic is implemented elsewhere to maintain the security and correctness of the key derivation process.
crypto/ledger/ledger_test.go (1)
- 19-21: The change from
NewParamstoNewFundraiserParamsinTestPublicKeyUnsafeis consistent with the refactor and the pull request's goal to streamline key generation and account recovery.simapp/simd/cmd/testnet.go (1)
- 276-279: The addition of
sdk.GetFullBIP44Path()as an argument totestutil.GenerateSaveCoinKeyis consistent with the refactor to remove global configuration for address coin type and purpose. Ensure that thesdk.GetFullBIP44Path()function is available and correctly returns the full BIP44 path as expected in this context.testutil/key.go (3)
14-20: The update to use
sdk.GetFullBIP44Path()directly aligns with the refactor to remove global configuration dependency.37-43: The addition of the
hdPathparameter toGenerateSaveCoinKeyfunction signature is a good practice to allow more flexibility in specifying the BIP44 path.64-72: The logic to handle both generation and recovery of a key based on the provided mnemonic is correctly implemented.
testutil/key_test.go (5)
21-21: The use of
types.CoinTypedirectly aligns with the refactor to remove global configuration. Ensure thattypes.CoinTypeis correctly defined and accessible in the context where this test is run.35-35: The addition of
types.GetFullBIP44Path()as an argument toGenerateSaveCoinKeyis consistent with the refactor. Verify thattypes.GetFullBIP44Path()is implemented correctly and provides the expected BIP44 path.61-61: The addition of
types.GetFullBIP44Path()as an argument toGenerateSaveCoinKeyis consistent with the refactor. Verify thattypes.GetFullBIP44Path()is implemented correctly and provides the expected BIP44 path.65-65: The test for the overwrite functionality of
GenerateSaveCoinKeywithoverwrite=falseis important to ensure that existing keys are not overwritten unintentionally. This test should fail if an attempt is made to overwrite an existing key without setting the flag to true.69-69: The test for the overwrite functionality of
GenerateSaveCoinKeywithoverwrite=trueis important to ensure that existing keys can be intentionally overwritten when required. This test should pass if the flag is set to true and the key is successfully overwritten.testutil/network/network.go (1)
- 458-458: The addition of
sdk.GetFullBIP44Path()as an argument totestutil.GenerateSaveCoinKeyaligns with the refactor to remove global configuration for address coin type and purpose. Ensure that theGenerateSaveCoinKeyfunction has been updated to handle this new argument and that all parts of the code that interact with this function are updated to pass the correct BIP44 path.types/config_test.go (1)
- 20-22: The test case
TestConfig_SetTxEncoderis using the deprecatedsdk.NewConfig()andSeal()method. However, since the test functionsTestConfig_SetPurposeandTestConfig_SetCoinTypehave been removed, this test case should also be updated or removed if it's no longer relevant due to the refactor.
|
nit, this does not close the mentioned issue. We should just reference it. |
| @@ -128,18 +127,6 @@ func (config *Config) SetFullFundraiserPath(fullFundraiserPath string) { | |||
| config.fullFundraiserPath = fullFundraiserPath | |||
| } | |||
|
|
|||
| // Set the BIP-0044 Purpose code on the config | |||
| func (config *Config) SetPurpose(purpose uint32) { | |||
There was a problem hiding this comment.
Have you checked on sourcegraph if those were used?
There was a problem hiding this comment.
It is being used in some chains, but as far as I can see, all of them use types.Purpose directly. GetPurpose is only used on tests
There was a problem hiding this comment.
So Get/SetPurpose is redundant, since it is never really used
There was a problem hiding this comment.
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 entry correctly reflects the changes described in the summary regarding the removal of global configuration for coin type and purpose. Ensure that the associated code changes are also included in the pull request.
Description
Closes: #18594
ref #7448
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.mdmake lintandmake testReviewers 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
Refactor
Tests
Documentation
typesmodule.Chores