fix: apply comments from the c4 audits #132
Conversation
WalkthroughThe pull request introduces modifications across multiple files in the x/opchild and x/ophost modules, focusing on enhancing error handling, vote extension processing, and validation mechanisms. Changes include updates to oracle vote validation, token deposit processes, and bridge configuration checks. The modifications aim to improve the robustness of the system by adding more precise error handling and refining the logic for processing oracle-related operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgServer
participant OracleHandler
participant Validator
Client->>MsgServer: Initiate Token Deposit
MsgServer->>MsgServer: Check Bridge Configuration
alt Bridge Not Found
MsgServer-->>Client: Return Bridge Not Found Error
else Bridge Exists
MsgServer->>OracleHandler: Process Deposit
OracleHandler->>Validator: Validate Vote Extensions
Validator-->>OracleHandler: Return Validated Votes
OracleHandler-->>MsgServer: Confirm Deposit
MsgServer-->>Client: Deposit Successful
end
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 49.41% 49.40% -0.01%
==========================================
Files 57 57
Lines 4256 4271 +15
==========================================
+ Hits 2103 2110 +7
- Misses 1720 1723 +3
- Partials 433 438 +5
|
![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
🧹 Nitpick comments (1)
x/opchild/keeper/oracle.go (1)
96-96: Maintain Code ConsistencyLine 96:
There is an unnecessary blank line after the error check:
if err != nil { return err } - prices, err := k.voteAggregator.AggregateOracleVotes(sdkCtx, votes)Consider removing the extra blank line to maintain code consistency and readability.
Apply this diff:
if err != nil { return err } -prices, err := k.voteAggregator.AggregateOracleVotes(sdkCtx, votes) +prices, err := k.voteAggregator.AggregateOracleVotes(sdkCtx, votes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
x/opchild/keeper/msg_server.go(1 hunks)x/opchild/keeper/msg_server_test.go(1 hunks)x/opchild/keeper/oracle.go(1 hunks)x/opchild/keeper/oracle_test.go(2 hunks)x/opchild/keeper/validator.go(2 hunks)x/opchild/l2connect/aggregator.go(1 hunks)x/opchild/l2connect/utils.go(5 hunks)x/ophost/keeper/msg_server.go(1 hunks)x/ophost/types/error.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/opchild/keeper/validator.go
[warning] 21-21: x/opchild/keeper/validator.go#L21
Added line #L21 was not covered by tests
[warning] 42-42: x/opchild/keeper/validator.go#L42
Added line #L42 was not covered by tests
x/opchild/l2connect/utils.go
[warning] 46-46: x/opchild/l2connect/utils.go#L46
Added line #L46 was not covered by tests
[warning] 57-58: x/opchild/l2connect/utils.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 70-70: x/opchild/l2connect/utils.go#L70
Added line #L70 was not covered by tests
[warning] 75-75: x/opchild/l2connect/utils.go#L75
Added line #L75 was not covered by tests
[warning] 80-80: x/opchild/l2connect/utils.go#L80
Added line #L80 was not covered by tests
[warning] 93-93: x/opchild/l2connect/utils.go#L93
Added line #L93 was not covered by tests
[warning] 98-98: x/opchild/l2connect/utils.go#L98
Added line #L98 was not covered by tests
[warning] 110-110: x/opchild/l2connect/utils.go#L110
Added line #L110 was not covered by tests
[warning] 114-114: x/opchild/l2connect/utils.go#L114
Added line #L114 was not covered by tests
[warning] 123-123: x/opchild/l2connect/utils.go#L123
Added line #L123 was not covered by tests
[warning] 128-128: x/opchild/l2connect/utils.go#L128
Added line #L128 was not covered by tests
x/ophost/keeper/msg_server.go
[warning] 230-230: x/ophost/keeper/msg_server.go#L230
Added line #L230 was not covered by tests
[warning] 232-233: x/ophost/keeper/msg_server.go#L232-L233
Added lines #L232 - L233 were not covered by tests
🔇 Additional comments (12)
x/ophost/types/error.go (1)
24-24: LGTM! Well-structured error definition.The new error
ErrBridgeNotFoundfollows the established pattern and uses an appropriate error code.x/opchild/l2connect/utils.go (4)
49-51: Initialize Variables for Accurate Vote ProcessingInitializing
totalVP,seenValidators, andvalidVotesenhances clarity and prepares for accurate vote processing.Lines 49-51:
totalVP = totalBondedTokens.Int64() seenValidators := make(map[string]bool) validVotes := make([]cometabci.ExtendedVoteInfo, 0, len(extCommit.Votes))This setup is appropriate for maintaining state during vote validation.
56-61: Prevent Duplicate Votes from ValidatorsThe logic to prevent duplicate votes ensures each validator's vote is counted only once.
Lines 56-61:
if strAddr := valConsAddr.String(); seenValidators[strAddr] { // ignore duplicate votes continue } else { seenValidators[strAddr] = true }This prevents potential double-counting and enhances the integrity of the voting process.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: x/opchild/l2connect/utils.go#L57-L58
Added lines #L57 - L58 were not covered by tests
117-118: Accumulate Valid Votes and Voting PowerCollecting valid votes and summing their voting power are crucial steps.
Lines 117-118:
validVotes = append(validVotes, vote) sumVP += power.Int64()This implementation correctly prepares the data for the super-majority check.
134-134: Return Validated VotesLine 134:
return validVotes, nilReturning the slice of validated votes aligns with the updated function signature and enables further processing downstream.
x/opchild/l2connect/aggregator.go (1)
23-26: Update Function Signature and Logic inGetOracleVotesThe function
GetOracleVotesnow acceptsextendedVotes []cometabci.ExtendedVoteInfoinstead ofextendedCommitInfo cometabci.ExtendedCommitInfo.Lines 23-26:
func GetOracleVotes( veCodec connectcodec.VoteExtensionCodec, extendedVotes []cometabci.ExtendedVoteInfo, ) ([]connectaggregator.Vote, error) { votes := make([]connectaggregator.Vote, len(extendedVotes)) for i, voteInfo := range extendedVotes {This change ensures compatibility with the updated return value of
ValidateVoteExtensionsand correctly processes the votes. The loop and decoding logic have been appropriately adjusted.x/opchild/keeper/oracle.go (2)
87-88: CaptureextendedVotesfromValidateVoteExtensionsLines 87-88:
extendedVotes, err := l2connect.ValidateVoteExtensions(sdkCtx, k.HostValidatorStore, h-1, hostChainID, extendedCommitInfo) if err != nil { return err }By capturing
extendedVotes, the code now utilizes the validated votes returned byValidateVoteExtensions, which is essential for subsequent processing.
92-94: PassextendedVotestoGetOracleVotesLines 92-94:
votes, err := l2connect.GetOracleVotes(k.veCodec, extendedVotes) if err != nil { return err }Updating the parameter to
extendedVotesaligns with the updated function signature ofGetOracleVotes, ensuring consistency and correct data flow.x/opchild/keeper/validator.go (1)
41-42: Add test coverage for panic scenarios in GetValidatorByConsAddr.Similar to GetValidator, this critical error path needs test coverage to ensure proper handling of unexpected errors.
Run this script to verify test coverage:
✅ Verification successful
The review comment is correct - panic scenario needs test coverage
The test file
x/opchild/keeper/validator_test.gocontains aTest_GetValidatorByConsAddrfunction that covers the happy path and "not found" scenarios, but lacks coverage for the panic path when unexpected errors occur. This matches the pattern used inGetValidatorwhere similar error handling exists.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for GetValidatorByConsAddr panic scenario rg -A 5 'func TestGetValidatorByConsAddr'Length of output: 43
Script:
#!/bin/bash # Find the implementation of GetValidatorByConsAddr ast-grep --pattern 'func ($_ *$_) GetValidatorByConsAddr($_) $_' # Find any test files that might contain tests for this function fd -e go -E vendor test # Look for similar panic scenario tests in the codebase rg -g '**/*_test.go' -B 2 -A 5 'panic\('Length of output: 2161
Script:
#!/bin/bash # Check the implementation in validator.go cat x/opchild/keeper/validator.go # Check the test file cat x/opchild/keeper/validator_test.go # Look for any GetValidatorByConsAddr tests with different naming patterns rg -g 'validator_test.go' 'ByConsAddr'Length of output: 10464
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: x/opchild/keeper/validator.go#L42
Added line #L42 was not covered by testsx/opchild/keeper/msg_server.go (1)
463-463: LGTM! Improved withdrawal condition logic.The condition for initiating withdrawals has been tightened to only trigger when both deposit and hook have failed AND the coin amount is positive. This is a security improvement that prevents unnecessary withdrawals.
x/opchild/keeper/oracle_test.go (1)
53-53: LGTM! Function rename maintains functionality.The function rename from
getSlinkytogetConnectis consistent with the codebase changes while preserving the test logic.x/opchild/keeper/msg_server_test.go (1)
658-658: LGTM! Consistent function rename.The update to use
getConnectinstead ofgetSlinkymaintains consistency with the changes in oracle_test.go.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/ophost/keeper/msg_server_test.go (1)
183-189: LGTM! Consider enhancing the error assertion.The test case effectively verifies error handling for non-existent bridges. To improve clarity, consider asserting the specific error type.
- require.Error(t, err) + require.ErrorIs(t, err, types.ErrBridgeNotFound)
Description
Closes: #XXXX
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 changeReviewers 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...
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
The release focuses on improving system reliability and error handling across multiple components of the blockchain infrastructure.