fix: check for duplicates in observer set #2672
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThis update introduces significant enhancements to the Zeta Chain project's observer management and authorization processes. Key improvements include robust error handling for adding and updating observers, validation checks for duplicate entries, and the introduction of new error types. These changes aim to enhance the reliability and integrity of the system, ensuring that observer management and authorization processes operate without inconsistencies or unauthorized actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MsgServer
participant ObserverKeeper
User->>MsgServer: AddObserver()
MsgServer->>ObserverKeeper: AddObserverToSet()
ObserverKeeper-->>MsgServer: Return Error (if any)
MsgServer-->>User: Return response (success/error)
sequenceDiagram
participant User
participant MsgServer
participant ObserverKeeper
User->>MsgServer: UpdateObserver()
MsgServer->>ObserverKeeper: UpdateObserverAddress()
ObserverKeeper-->>MsgServer: Return Error (if any)
MsgServer-->>User: Return response (success/error)
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 Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2672 +/- ##
========================================
Coverage 71.25% 71.25%
========================================
Files 351 351
Lines 18982 18993 +11
========================================
+ Hits 13525 13534 +9
- Misses 4853 4856 +3
+ Partials 604 603 -1
|
…server-set' into trailofbits-4-duplicate-check-observer-set # Conflicts: # changelog.md
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
…server-set' into trailofbits-4-duplicate-check-observer-set # Conflicts: # x/observer/keeper/observer_set_test.go
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
The observer set is also set during genesis , but I think we can assume the genesis state to be valid , as Validate should already have been run on the genesisState |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (2)
x/observer/types/observer_set.go (1)
18-20: Clarify the validation comment.The comment block could be more descriptive by explaining the implications of invalid or duplicate observer addresses.
- // Validate observer set verifies that the observer set is valid - // - All observer addresses are valid - // - No duplicate observer addresses + // Validate ensures the observer set contains only valid and unique addresses. + // - Verifies each observer address is in a valid Bech32 format. + // - Ensures no duplicate addresses exist in the observer set.x/observer/types/observer_set_test.go (1)
13-38: Enhance test case descriptions.The test case names are clear, but consider adding comments to explain the purpose of each case for future maintainers.
+ // Test cases for ObserverSet validation + // Ensures proper error handling for duplicates, invalid addresses, and valid scenarios.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- changelog.md (1 hunks)
- x/observer/keeper/msg_server_add_observer.go (1 hunks)
- x/observer/keeper/msg_server_add_observer_test.go (1 hunks)
- x/observer/keeper/msg_server_update_observer_test.go (1 hunks)
- x/observer/keeper/observer_set.go (3 hunks)
- x/observer/keeper/observer_set_test.go (5 hunks)
- x/observer/types/errors.go (1 hunks)
- x/observer/types/observer_set.go (2 hunks)
- x/observer/types/observer_set_test.go (1 hunks)
Additional context used
Path-based instructions (8)
x/observer/types/observer_set.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/types/observer_set_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/msg_server_add_observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/types/errors.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/observer_set.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/msg_server_add_observer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/observer_set_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/msg_server_update_observer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (11)
x/observer/types/observer_set_test.go (1)
23-25: Ensure error message consistency.Check that the error message returned in the code matches the expected error in the test case for consistency.
Verification successful
Error Message Consistency Verified
The error message for
ErrDuplicateObserveris consistently defined as "observer already exists" and is correctly used across the codebase. The test cases appropriately check for this error type, ensuring consistency. No changes are needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of error messages for duplicate observer errors. # Test: Search for the error message definition and usage. Expect: Consistent error messages. rg --type go --word-regexp "ErrDuplicateObserver"Length of output: 797
x/observer/types/errors.go (1)
52-53: LGTM! Enhanced error handling.The introduction of
ErrDuplicateObserverandErrObserverNotFoundimproves clarity and specificity in error reporting.x/observer/keeper/msg_server_add_observer_test.go (1)
55-83: LGTM! Enhanced test coverage.The addition of the test case for duplicate observers effectively ensures that the system handles duplicates correctly. Consider adding more edge cases if applicable.
x/observer/keeper/observer_set_test.go (5)
Line range hint
39-70: Structured comments and error handling enhance test readability and robustness.The addition of "ARRANGE", "ACT", and "ASSERT" comments improves the readability of the test. The error handling ensures that the addition of observers is correctly validated.
58-70: Test case ensures proper handling of empty observer set.This test case verifies that an observer can be added when the observer set does not exist, ensuring robustness in the initialization process.
72-87: New test case effectively validates duplicate observer prevention.The test case ensures that the system correctly handles attempts to add duplicate observers, enhancing the validation logic.
121-147: Enhanced error handling in observer address update tests.The new test cases improve error handling by checking for specific errors, such as
types.ErrObserverSetNotFoundandtypes.ErrDuplicateObserver, ensuring robust validation.
156-156: Comprehensive test coverage for observer management.The test cases provide thorough coverage for various scenarios, ensuring that observer management functions correctly under different conditions.
x/observer/keeper/msg_server_update_observer_test.go (2)
76-129: New test case ensures prevention of duplicate addresses in tombstoned observer updates.The test case effectively checks for
types.ErrDuplicateObserverwhen updating a tombstoned observer with a duplicate address, enhancing system integrity.
130-130: Comprehensive test coverage for observer update scenarios.The test cases ensure that the observer update functionality handles various error scenarios gracefully, providing robust validation.
changelog.md (1)
19-19: Changelog accurately reflects new validation features.The updates to the changelog provide clear documentation of the new validation features for the observer set and authorization list, enhancing transparency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…server-set' into trailofbits-4-duplicate-check-observer-set
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 13340122 | Triggered | Generic High Entropy Secret | d2f2074 | zetaclient/chains/solana/signer/signer_test.go | View secret |
| 13340122 | Triggered | Generic High Entropy Secret | d2f2074 | zetaclient/chains/solana/signer/signer_test.go | View secret |
| 13392123 | Triggered | Generic High Entropy Secret | d2f2074 | zetaclient/chains/solana/signer/signer_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Description
Closes https://github.com/zeta-chain/security/issues/85
zetacore: check for duplicate addresses when adding or updating the observer set #2670zetacore: fix events for add observer #2671How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation