feat(core,runtime): add address codecs in environment #21905
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
core/appmodule/v2/environment.go (2)
31-33: LGTM: New address codec fields are well-structured and align with PR objectives.The new fields
AddressCodec,ValidatorAddressCodec, andConsensusAddressCodecare correctly added to theEnvironmentstruct. They align with the PR objectives to introduce address codecs into the environment. The field names are clear, follow Go naming conventions, and use appropriate types from theaddresspackage.Consider adding a brief comment above these new fields to explain their purpose and usage within the
Environmentstruct. This would enhance the documentation and make it easier for other developers to understand the role of these address codecs.
Potential Inconsistencies Detected with the New Address Codec Fields
The introduction of
AddressCodec,ValidatorAddressCodec, andConsensusAddressCodecinEnvironmenthas widespread implications across the codebase. Numerous functions and modules interact with theEnvironmentstruct and may require updates to handle these new codec fields appropriately. It's essential to review and modify each affected area to ensure consistency and prevent potential runtime issues.
- Modules and Functions to Review:
- Slashing Keeper
- Group Keeper
- Bank Keeper
- Authz Keeper
- Distribution Keeper
- Mint Keeper
- Consensus Keeper
- And many others as indicated by the shell script output.
🔗 Analysis chain
Line range hint
1-33: Overall changes align well with PR objectives and maintain code quality.The modifications to
environment.gosuccessfully introduce address codecs into theEnvironmentstruct as intended. The changes are minimal, focused, and well-integrated into the existing code structure. They comply with the Uber Go Style Guide and maintain the overall code quality.These additions enhance the struct's capability to manage different types of address encodings, which aligns perfectly with the PR objectives discussed in the Slack conversation.
To ensure these changes don't introduce any breaking changes or inconsistencies, please run the following verification script:
This script will help identify any potential areas in the codebase that might need to be updated to accommodate these new fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the new address codec fields in other files # Test: Search for references to the new fields echo "Checking for references to new address codec fields:" rg --type go -i '(AddressCodec|ValidatorAddressCodec|ConsensusAddressCodec)' # Test: Check if there are any existing methods or functions that might need to be updated echo "Checking for potential methods or functions that might need updates:" rg --type go -i 'func.*Environment'Length of output: 231229
runtime/v2/module.go (1)
182-184: LGTM: Address codec parameters and fields added correctly.The new address codec parameters and fields have been added correctly to the
ProvideEnvironmentfunction and theappmodulev2.Environmentstruct. The changes are consistent with the PR objectives and maintain backward compatibility.Consider adding a brief comment above the
ProvideEnvironmentfunction to explain the purpose of the new address codec parameters and how they enhance the environment's functionality.Also applies to: 224-226
runtime/module.go (1)
258-260: LGTM: Address codec parameters added correctly.The new parameters
addressCodec,valAddressCodec, andconsAddressCodechave been added to theProvideEnvironmentfunction, and they are correctly used in theEnvWithAddressCodecscall. This change implements a more modular approach to address handling within the application environment.Consider adding a brief comment above the
ProvideEnvironmentfunction to explain the purpose of these new address codec parameters and how they enhance the environment configuration.Also applies to: 282-282
runtime/environment.go (1)
149-150: Simplify error message formattingYou can simplify the error message formatting by including the space within the format string rather than concatenating it to the prefix.
Apply this diff to adjust the error messages:
func (f failingAddressCodec) StringToBytes(text string) ([]byte, error) { - return nil, fmt.Errorf("%saddress codec not set", f.prefix+" ") + return nil, fmt.Errorf("%s address codec not set", f.prefix) } func (f failingAddressCodec) BytesToString(bz []byte) (string, error) { - return "", fmt.Errorf("%saddress codec not set", f.prefix+" ") + return "", fmt.Errorf("%s address codec not set", f.prefix) }Also applies to: 153-154
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
🔇 Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- core/appmodule/v2/environment.go (2 hunks)
- go.mod (1 hunks)
- runtime/environment.go (4 hunks)
- runtime/module.go (3 hunks)
- runtime/v2/module.go (3 hunks)
- simapp/go.mod (1 hunks)
- tests/go.mod (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
core/appmodule/v2/environment.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/environment.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments not posted (9)
core/appmodule/v2/environment.go (1)
4-4: LGTM: Import statement is correct and necessary.The new import for the
addresspackage is correctly placed and necessary for the new address codec fields. It follows the Uber Go Style Guide for import formatting.runtime/v2/module.go (2)
17-17: LGTM: New import added correctly.The new import for the address package is correctly placed and follows the Uber Go Style Guide for import grouping and ordering.
182-184: Verify the impact of address codec changes on the codebase.The changes to
ProvideEnvironmentfunction andappmodulev2.Environmentstruct are well-implemented. However, it's important to ensure that these changes don't break existing functionality or require updates in other parts of the codebase.Please run the following script to check for any potential issues:
This script will help identify areas of the codebase that might need updates or verification due to the new address codec changes.
Also applies to: 224-226
runtime/module.go (3)
15-15: LGTM: New import added correctly.The new import
"cosmossdk.io/core/address"is correctly added and placed in alphabetical order within the import block. This import is necessary for the new address codec parameters in theProvideEnvironmentfunction.
258-260: Summary of changes: Address codecs added to environment configuration.The changes to
ProvideEnvironmentfunction enhance the modularity of address handling in the application environment. The new address codec parameters allow for more flexible and type-safe address management. These changes align with the PR objectives of introducing address codecs into the environment.Key points:
- New address codec parameters added correctly.
EnvWithAddressCodecsused to incorporate the new parameters.- Import statement added for the new
addresspackage.Next steps:
- Verify and update all callers of
ProvideEnvironmentin the codebase.- Consider adding brief documentation for the new parameters.
- Ensure that these changes are thoroughly tested, especially in integration scenarios.
Also applies to: 282-282
258-260: Verify impact on other parts of the codebase.The changes to
ProvideEnvironmentfunction signature will require updates in other parts of the codebase where this function is called. Please ensure that all callers of this function are updated to provide the new address codec parameters.Run the following script to identify potential places that need to be updated:
✅ Verification successful
Run the following corrected script to identify potential callers of
ProvideEnvironmentand related wire definitions:
Verification Successful: No additional updates needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential callers of ProvideEnvironment that might need updating # Search for function calls to ProvideEnvironment echo "Potential callers of ProvideEnvironment:" rg --type go "ProvideEnvironment\s*\(" -g '!runtime/module.go' # Search for wire definitions that might need updating echo "\nPotential wire definitions that might need updating:" rg --type go "provide.*environment"i -g '!runtime/module.go'Length of output: 337
Script:
#!/bin/bash # Description: Find potential callers of ProvideEnvironment that might need updating # Search for function calls to ProvideEnvironment excluding runtime/module.go echo "Potential callers of ProvideEnvironment:" rg --type go "ProvideEnvironment\s*\(" -g '!runtime/module.go' # Search for wire definitions that might need updating echo "\nPotential wire definitions that might need updating:" rg --type go "provide.*environment" -i -g '!runtime/module.go'Length of output: 1240
go.mod (1)
187-187: LGTM: New replacement directive forcosmossdk.io/coreadded.The addition of
cosmossdk.io/core => ./coreis consistent with other local replacements in this file. This change allows the project to use a local version of the core module, which is likely necessary for implementing the address codecs mentioned in the PR objectives.To ensure this change doesn't introduce any unintended consequences, please run the following verification script:
tests/go.mod (1)
240-240: LGTM: New replacement directive forcosmossdk.io/coreadded.The addition of
cosmossdk.io/core => ../coreis consistent with other local replacements in this file. This change allows the tests to use the local version of the core module, which is necessary for testing the new address codecs functionality mentioned in the PR objectives.simapp/go.mod (1)
246-246: LGTM: Local development setup for core moduleThe addition of the replacement directive for
cosmossdk.io/coreis consistent with other local development setups in this file. This change allows for easier testing and development of the core module locally.To ensure this change doesn't introduce any issues, please verify:
- The
../coredirectory exists and contains the expected core module code.- Any changes made to the core module are reflected when building or testing the simapp.
You can use the following script to verify the setup:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Will do if accepted. |
|
Not accepted to not leak val address and cons address in environment. |
Description
Proposal to add address codec in environment as discussed on Slack.
DNM until agreement.
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores
go.modfiles for local development and testing.