refactor: handle roundid gap when do growID #311
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant F as FeederManager
participant K as Keeper
F->>K: AppendPriceTR(ctx, tokenID, priceCommit, finalPrice.DetID)
alt AppendPriceTR fails
F->>K: GrowRoundID(ctx, tokenID, roundID)
else AppendPriceTR succeeds
Note right of F: Proceed without additional call
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/oracle/keeper/prices.go (1)
241-270: Address naked return and streamline clarity inGrowRoundID.
The linter flags the naked return on line 254 as a readability issue. While named returns are valid in Go, consider removing the naked return to improve clarity, especially in a 30+ line function.- return + return price, roundID🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 254-254:
naked return in funcGrowRoundIDwith 31 lines of code (nakedret)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/oracle/keeper/common/expected_keepers.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager.go(1 hunks)x/oracle/keeper/prices.go(2 hunks)x/oracle/keeper/prices_test.go(1 hunks)x/oracle/types/types.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
x/oracle/keeper/prices.go
[failure] 254-254:
naked return in func GrowRoundID with 31 lines of code (nakedret)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-unit-e2e
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (7)
x/oracle/keeper/prices.go (2)
213-216: Consider test coverage for early return with nil DetID.
Currently, the code skips further processing ifdetIDisNilDetID. If this is intentional, ensure there's test coverage confirming that no additional side effects are needed.
218-222: Double-check skipping post-processing for empty price.
Similar to theNilDetIDcheck, this early return could bypass post-processing steps if thePricefield is empty. Confirm that this behavior aligns with requirements and is tested.x/oracle/types/types.go (1)
70-71: Validate the use of an empty string forNilDetID.
Storing a nil/empty identifier as""is simple, but confirm that it won’t introduce confusion or collisions with legitimate empty strings.x/oracle/keeper/prices_test.go (1)
92-124: Good coverage forGrowRoundID.
This test thoroughly checks multiple scenarios of the function, including whennextRoundIDis less than, equal to, or greater than the stored value.x/oracle/keeper/common/expected_keepers.go (1)
32-32:❓ Verification inconclusive
Breaking change: GrowRoundID method signature updated.
The addition of the
nextRoundIDparameter aligns with the PR objective to handle roundID gaps more effectively. However, this is a breaking change that requires all implementations of theKeeperOracleinterface to be updated.Let's verify all implementations of the interface have been updated:
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of GrowRoundID to ensure they match the new signature. # Search for GrowRoundID implementations ast-grep --pattern 'func $_ GrowRoundID(ctx sdk.Context, tokenID uint64) $$$'Length of output: 79
I'll generate an additional verification script to check for both the new and any potentially outdated implementations of
GrowRoundID. This will clarify whether all implementations have been updated to include thenextRoundIDparameter.Once you run this script, please review the output. If implementations with the old signature are found, they must be updated. Otherwise, if only the updated signature is present, no further changes are needed.
Action Required: Update Implementations of GrowRoundID
The updated signature for
GrowRoundID(ctx sdk.Context, tokenID, nextRoundID uint64) (price string, roundID uint64)is a breaking change. Please ensure that all implementations of theKeeperOracleinterface include the newnextRoundIDparameter. The initial verification using theast-grepcommand did not provide output, so use the above script to manually verify that no outdated implementations exist.
🏁 Script executed:
#!/bin/bash # Description: Verify the GrowRoundID method signature across the codebase. # This script searches for implementations with the new signature (including nextRoundID) # and also checks for any implementations missing the parameter. echo "Searching for GrowRoundID implementations with the new signature (with nextRoundID)..." rg "func\s+.*GrowRoundID\(ctx sdk.Context, tokenID, nextRoundID uint64" -n . echo "------------------------------------------" echo "Searching for any GrowRoundID implementations that might be missing nextRoundID..." rg "func\s+.*GrowRoundID\(ctx sdk.Context, tokenID(?!,)" -n .Length of output: 1033
BREAKING CHANGE Verification: GrowRoundID Signature Updated
The implementation in x/oracle/keeper/prices.go (line 241) has been updated to the new signature:
func (k Keeper) GrowRoundID(ctx sdk.Context, tokenID, nextRoundID uint64) (price string, roundID uint64) {Our initial search confirms this updated implementation. However, our attempt to search for outdated signatures using look-around regex failed. To double-check that no implementations are still using the old signature (i.e., missing the
nextRoundIDparameter), please run the following script:#!/bin/bash echo "Searching for implementations of GrowRoundID that don't mention nextRoundID parameter..." rg "func.*GrowRoundID\\(.*tokenID" -n . | grep -v "nextRoundID"If this search returns no results, it confirms that all implementations of the
KeeperOracleinterface have been updated as required by this breaking change.Please verify the output of the above script to ensure that no legacy implementations remain.
x/oracle/keeper/feedermanagement/feedermanager.go (2)
278-278: LGTM: GrowRoundID call updated with roundID parameter.The call has been correctly updated to match the new interface signature, passing the current round's ID as the expected next round ID.
285-288:✅ Verification successful
Verify error handling for roundID gap.
The new error handling for AppendPriceTR failure looks good. It attempts to grow the roundID when price appending fails, which aligns with the PR objective to handle roundID gaps. However, we should verify if this is the only place where such gaps can occur.
Let's search for other AppendPriceTR calls that might need similar error handling:
🏁 Script executed:
#!/bin/bash # Description: Find all AppendPriceTR calls to ensure consistent error handling. # Search for AppendPriceTR calls ast-grep --pattern 'AppendPriceTR($$$)'Length of output: 41
Action Required: Double-Check AppendPriceTR Calls for Consistent Error Handling
The current implementation correctly handles an AppendPriceTR failure by attempting to grow the roundID when a gap is detected, which aligns with the PR’s objectives. However, to be certain that this is the only location requiring such error handling, please perform a comprehensive repository-wide search for "AppendPriceTR" (for example, using the above rg command) to ensure there are no additional calls that might be missing similar safeguards.
🏁 Script executed:
#!/bin/bash # Using rg to search for any occurrences of "AppendPriceTR" in the entire repository rg "AppendPriceTR" .Length of output: 541
Action Required: Confirm if any additional AppendPriceTR calls need similar error handling.
The initial search confirmed three locations referencing AppendPriceTR. The error handling we reviewed in
feedermanager.goappears to cover the case where the function returns false due to a roundID gap. Meanwhile, the call inprices.gosimply delegates to AppendPriceTR without additional error handling, likely because it is part of the internal implementation.Please verify the output of the
GrowRoundIDsearch to confirm no other call sites require similar error handling. Once verified, the current approach infeedermanager.gois considered appropriate within its context.
🏁 Script executed:
#!/bin/bash # Using rg to search for any occurrences of "GrowRoundID" to understand the usage context in the repository. rg "GrowRoundID" .Length of output: 978
AppendPriceTR Error Handling Verification Approved
After verifying across the repository, we confirmed that the only instance where error handling is applied to an AppendPriceTR call is in
x/oracle/keeper/feedermanagement/feedermanager.go. The other instances inprices.goand the interface inexpected_keepers.goappear to be part of the internal implementation and do not require additional error handling. The current approach—invokingGrowRoundIDupon AppendPriceTR failure—is appropriate and consistent with the intended behavior for handling roundID gaps.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/oracle/keeper/prices.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-unit-e2e
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
x/oracle/keeper/prices.go (2)
213-222: LGTM! Efficient optimization of post-processing logic.The early returns for
NilDetIDand empty price cases improve efficiency by avoiding unnecessary post-processing while maintaining the core functionality.
241-244: LGTM! Improved roundID handling logic.The changes effectively prevent incorrect roundID persistence by:
- Using expected nextRoundID instead of incrementing state ID
- Handling cases where stored nextRoundID differs from expected
Also applies to: 249-260
leonz789
force-pushed
the
ref/oracle-roundid
branch
from
5a36f29 to
5feadcf
Compare
Description
use 'expected' roundID to update next round when do growID instead of increased on state ID.
Both approach should work fine for the 'GrowRoundID' case, this refactored one get rid of the 'impossible case' that increasing roundID by mistake which will lead 'incorrect' roundID forever.
supersede #295
Closes #XXX
Summary by CodeRabbit
Summary by CodeRabbit