refactor(x/evidence): accept address codec in constructor #21859
Conversation
tac0turtle
requested review from
alpe,
akhilkumarpilli,
a team,
facundomedica and
julienrbrt
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve enhancements to the Changes
Suggested labels
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
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -23,7 +23,7 @@ | |||
| // It's still ongoing discussion how should we treat and slash attacks with | |||
| // premeditation. So for now we agree to treat them in the same way. | |||
| case comet.LightClientAttack, comet.DuplicateVote: | |||
| evidence := types.FromABCIEvidence(evidence, k.stakingKeeper.ConsensusAddressCodec()) | |||
| evidence := types.FromABCIEvidence(evidence, k.consensusAddressCodec) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
x/evidence/keeper/abci.go (1)
26-26: LGTM! Consider enhancing error handling.The change aligns well with the PR objective of accepting the address codec in the constructor, improving consistency in how the address codec is used throughout the codebase. The overall logic of the function remains intact, and the modification doesn't introduce new panic scenarios.
Consider wrapping the
FromABCIEvidencecall in a separate function that includes error handling. This would improve the robustness of the code:func (k Keeper) convertABCIEvidence(evidence comet.Evidence) (types.Evidence, error) { converted, err := types.FromABCIEvidence(evidence, k.consensusAddressCodec) if err != nil { return nil, fmt.Errorf("failed to convert ABCI evidence: %w", err) } return converted, nil }Then, you can use this function in the
BeginBlocker:evidence, err := k.convertABCIEvidence(evidence) if err != nil { return err } err = k.handleEquivocationEvidence(ctx, evidence) if err != nil { return err }This approach would provide more detailed error information and make the code more maintainable.
x/evidence/depinject.go (1)
36-40: LGTM! Consider adding a comment for the new field.The addition of
ConsensusAddressCodecaligns with the PR objective and follows the existing naming conventions. To improve clarity, consider adding a brief comment explaining the purpose of this new field, similar to other fields in the struct.AddressCodec address.Codec - ConsensusAddressCodec address.Codec + ConsensusAddressCodec address.Codec // Codec for encoding/decoding consensus addressesx/evidence/keeper/keeper.go (1)
26-32: LGTM! Consider adding a comment for the new field.The addition of
consensusAddressCodecto theKeeperstruct is consistent with the PR objectives. The field is properly typed and follows the existing naming convention.Consider adding a brief comment explaining the purpose of the
consensusAddressCodecfield, similar to other fields in the struct. This would improve code readability and maintainability.x/evidence/keeper/keeper_test.go (1)
114-114: LGTM! Consider adding a comment for clarity.The addition of
address.NewBech32Codec("cosmosvalcons")aligns with the PR objective of accepting an address codec in the constructor. This change enhances the test setup to include a consensus address codec.Consider adding a brief comment to explain the purpose of this new codec:
// Add consensus address codec for validator consensus addresses address.NewBech32Codec("cosmosvalcons"),
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (9)
- simapp/app.go (1 hunks)
- x/evidence/CHANGELOG.md (1 hunks)
- x/evidence/depinject.go (2 hunks)
- x/evidence/keeper/abci.go (1 hunks)
- x/evidence/keeper/infraction.go (1 hunks)
- x/evidence/keeper/keeper.go (2 hunks)
- x/evidence/keeper/keeper_test.go (1 hunks)
- x/evidence/testutil/expected_keepers_mocks.go (0 hunks)
- x/evidence/types/expected_keepers.go (0 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- x/evidence/testutil/expected_keepers_mocks.go
- x/evidence/types/expected_keepers.go
🧰 Additional context used
📓 Path-based instructions (7)
simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/evidence/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/keeper/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/keeper/infraction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments not posted (7)
x/evidence/depinject.go (2)
Line range hint
36-51: Summary: Changes align well with PR objectivesThe modifications in this file successfully implement the PR's objective of accepting an address codec in the constructor. The changes are minimal and focused, enhancing the
evidencemodule's functionality without introducing unnecessary complexity. The newConsensusAddressCodecfield inModuleInputsand its usage inProvideModulefunction are consistent with the existing code structure and naming conventions.
51-51: LGTM! Verify keeper.NewKeeper signature.The update to include
in.ConsensusAddressCodecin thekeeper.NewKeepercall aligns with the PR objective and corresponds to the new field inModuleInputs. The change is focused and minimal.To ensure consistency, please verify that the
keeper.NewKeeperfunction signature in the keeper package has been updated to accept this new parameter. Run the following script to check:✅ Verification successful
LGTM! The
keeper.NewKeeperfunction signature includesin.ConsensusAddressCodecas expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the keeper.NewKeeper function signature # Test: Search for the NewKeeper function definition ast-grep --lang go --pattern 'func NewKeeper($$$) $_ { $$$ }'Length of output: 56664
x/evidence/CHANGELOG.md (1)
35-35: LGTM: Changelog entry is accurate and well-formatted.The new changelog entry correctly follows the guidelines, accurately reflects the changes described in the PR objectives, and is placed in the appropriate "Api Breaking Changes" section. It clearly explains that
NewKeepernow accepts a consensus codec and the purpose of this change.x/evidence/keeper/keeper.go (3)
42-42: LGTM! Function signature updated correctly.The
NewKeeperfunction signature has been properly updated to include the newconsensusAddressCodecparameter. This change is consistent with the PR objectives and maintains the existing function structure and naming conventions.
46-53: LGTM! Keeper initialization updated correctly.The
NewKeeperfunction body has been properly updated to initialize the newconsensusAddressCodecfield in theKeeperstruct. This change is consistent with the PR objectives and maintains the existing structure of the function.
Line range hint
26-53: Overall changes look good and align with PR objectives.The changes to the
Keeperstruct andNewKeeperfunction successfully integrate the newconsensusAddressCodecfield, allowing the evidence module to accept a consensus codec in its constructor. This enhancement should improve consistency in how the address codec is utilized throughout the codebase.Key points:
- New
consensusAddressCodecfield added toKeeperstruct.NewKeeperfunction signature updated to accept the new parameter.- Proper initialization of the new field in the
NewKeeperfunction.These changes are well-implemented and maintain the existing code structure and conventions.
simapp/app.go (1)
445-451: LGTM: EvidenceKeeper initialization looks correct.The changes to the
evidenceKeeperinitialization look good. The new parameterapp.StakingKeeper.ConsensusAddressCodec()has been added, which aligns with the PR objective of accepting an address codec in the constructor. This change ensures consistency in how the address codec is used throughout the codebase.
tac0turtle
requested review from
JulianToledano,
aaronc and
testinginprod
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/evidence/keeper/infraction_test.go (1)
Line range hint
1-624: Consider adding a test case for the new address codec functionality.While the existing tests provide good coverage for the evidence module, it might be beneficial to add a specific test case that verifies the correct usage of the newly added consensus address codec. This would ensure that the new parameter is being utilized as expected within the evidence keeper.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- collections/naming_test.go (1 hunks)
- schema/indexer/start.go (1 hunks)
- schema/indexer/start_test.go (1 hunks)
- tests/integration/evidence/keeper/infraction_test.go (1 hunks)
- tools/hubl/internal/remote.go (0 hunks)
- x/evidence/keeper/keeper.go (2 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- tools/hubl/internal/remote.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evidence/keeper/keeper.go
🧰 Additional context used
📓 Path-based instructions (4)
collections/naming_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/indexer/start.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/indexer/start_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/evidence/keeper/infraction_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments not posted (8)
collections/naming_test.go (4)
36-36: Excellent addition of t.Helper()The addition of
t.Helper()is a great improvement. This call marks the function as a test helper, which enhances the clarity of test failure messages by pointing to the calling test function rather than the helper itself. This change aligns with Go testing best practices and will make debugging failed tests easier.
44-44: Consistent improvement with t.Helper()The addition of
t.Helper()here is consistent with the change inexpectKeyCodecName. This maintains a uniform approach across test helper functions, which is excellent for code readability and maintainability. Good job on applying this improvement consistently.
52-52: Completed set of t.Helper() improvementsThe addition of
t.Helper()toexpectKeyCodecNamescompletes the set of improvements to all test helper functions in this file. This consistent application oft.Helper()across all helper functions demonstrates attention to detail and adherence to Go testing best practices. These changes will collectively enhance the debugging experience when tests fail by providing more accurate error locations.
36-52: Overall improvement in test helper functionsThe changes made to this file consistently add
t.Helper()to all test helper functions (expectKeyCodecName,expectValueCodecName, andexpectKeyCodecNames). This improvement aligns with Go testing best practices and enhances the debugging experience by providing more accurate error locations in case of test failures.These modifications do not alter the core functionality of the tests but significantly improve their usability and maintainability. The consistent application of this change across all helper functions demonstrates a thorough approach to code improvement.
Great job on enhancing the test suite's effectiveness!
schema/indexer/start_test.go (2)
83-83: Excellent addition oft.Helper()!Adding
t.Helper()to thecallCommitfunction is a great improvement. This change aligns with Go best practices and the Uber Go Style Guide. It helps in providing more accurate line numbers in test failure reports, making it easier to identify the source of failures in the actual test function rather than in the helper.
Line range hint
1-236: Overall assessment: Positive improvement with no side effectsThe addition of
t.Helper()in thecallCommitfunction is the only change in this file. It's a positive improvement that enhances the test output without altering the test logic or coverage. This change is in line with Go best practices and doesn't introduce any risks or require additional modifications.schema/indexer/start.go (1)
199-199: LGTM: Improved function signature aligns with Uber Golang style guide.The change to the
unmarshalIndexerCustomConfigfunction signature is an improvement. By using the shorthand declaration for multiple parameters of the same type (cfg, expectedType interface{}), the code becomes more concise without sacrificing readability. This change aligns well with the Uber Golang style guide, which encourages clear and concise code.The function's logic remains unchanged, ensuring that its behavior and functionality are preserved.
tests/integration/evidence/keeper/infraction_test.go (1)
157-157: LGTM! Verify impact on address handling.The addition of
stakingKeeper.ConsensusAddressCodec()as a parameter toNewKeeperaligns with the PR objective of accepting an address codec in the constructor. This change enhances the flexibility of the evidence keeper by allowing it to use the consensus address codec from the staking keeper.To ensure this change doesn't introduce any inconsistencies, please run the following script to check for any other occurrences of
NewKeeperthat might need similar updates:✅ Verification successful
Verification Complete: No additional
NewKeeperinstances require updates. The impact on address handling has been thoroughly checked.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other NewKeeper calls in the codebase rg --type go 'NewKeeper\(' -A 10 -B 2Length of output: 100498
Description
this pr moves towards evidence accepting consensus codec to be consistent with how address codec is passed around
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
NewKeeperfunction to accept a consensus codec, improving address decoding.Bug Fixes
Documentation
Tests