fix(#319,#333): oracle quorum check + reserve tracker duplicate curre… - #459
Open
hartz0 wants to merge 4 commits into
Open
fix(#319,#333): oracle quorum check + reserve tracker duplicate curre…#459hartz0 wants to merge 4 commits into
hartz0 wants to merge 4 commits into
Conversation
…ve tracker duplicate currency guard Issue Pi-Defi-world#319 — update_rate now enforces max(min_signatures, MIN_ORACLE_SOURCE_FEEDS=3) sources when sources are provided. A single-validator submission with < required feeds is rejected with InsufficientOracleSources (#7009). Zero-source bypass is preserved. Issue Pi-Defi-world#333 — add_currency checks the stored list before push and panics with DuplicateCurrency (#8008) on a duplicate, preventing double-counting of reserves. Adds get_currencies() to expose the tracked list. Also: - Resolves DataKey merge conflict: both last_verify_call (rate-limit) and currencies (dedup) are present in the struct - Adds tests for both fixes in their respective test files - Regenerates docs/ERROR_CODES.md with DuplicateCurrency = 8008
📝 WalkthroughWalkthroughOracle ChangesOracle quorum enforcement
Reserve tracker error update
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The variant was referenced in assert_currency_registered() but never declared in the enum, causing a compile error whenever acbu_oracle is built. Adds the variant and its Display arm, then regenerates ERROR_CODES.md.
Contributor
Author
|
hello maintainer, please merge my pr |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
acbu_oracle/src/lib.rs (1)
434-455: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winClose the new guard and remove the obsolete quorum block.
The new
ifis never closed before the old check begins. This leavesupdate_rateopen and makes the followingpub fnitems invalid nested declarations, so the crate cannot compile.Proposed fix
if sources.len() > 0 { let min_sigs: u32 = env .storage() .instance() .get(&DATA_KEY.min_signatures) .unwrap(); let required = min_sigs.max(MIN_ORACLE_SOURCE_FEEDS); if sources.len() < required { env.panic_with_error(OracleError::InsufficientOracleSources); } - let min_sigs: u32 = env - .storage() - .instance() - .get(&DATA_KEY.min_signatures) - .unwrap(); - let required = min_sigs.max(MIN_ORACLE_SOURCE_FEEDS); - // The 0/1-source path below intentionally bypasses median/outlier - // aggregation, so the multi-source quorum floor only applies once - // there's more than one source to aggregate. - if sources.len() > 1 && sources.len() < required { - env.panic_with_error(OracleError::InsufficientOracleSources); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@acbu_oracle/src/lib.rs` around lines 434 - 455, Fix update_rate by removing the obsolete initial min-signatures/quorum block and ensuring the remaining multi-source quorum check is properly closed before the following pub fn declarations. Preserve the intended condition that only source counts greater than one and below the required threshold panic with InsufficientOracleSources.acbu_reserve_tracker/src/lib.rs (1)
24-31: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winConsolidate the duplicate
ReserveTrackerErrorvariants and regenerate the error catalog.
ReserveTrackerErrorcannot compile with duplicate variant names atacbu_reserve_tracker/src/lib.rs#L24-L31, while thedocs/ERROR_CODES.mdtable only documentsDuplicateCurrencyand omits the other reserve-tracker variants added in this block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@acbu_reserve_tracker/src/lib.rs` around lines 24 - 31, The ReserveTrackerError definitions contain duplicate variant names and codes, and the error catalog is incomplete. In acbu_reserve_tracker/src/lib.rs lines 24-31, consolidate the duplicate variants while preserving one unique definition per error and assign non-conflicting codes; then regenerate docs/ERROR_CODES.md line 192 to document every remaining reserve-tracker variant with its finalized code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acbu_oracle/src/lib.rs`:
- Around line 434-443: Replace the raw length-based source validation in the
surrounding oracle submission flow with uniquely identified, independently
authenticated feed or validator attestations; do not count caller-supplied i128
values as separate sources. Update the source representation and verification
logic so each attestation is authenticated and deduplicated by its
feed/validator identity before enforcing DATA_KEY.min_signatures and
MIN_ORACLE_SOURCE_FEEDS, then compute the median only from validated
attestations.
In `@acbu_oracle/tests/test.rs`:
- Line 708: Replace the broad #[should_panic] annotations at
acbu_oracle/tests/test.rs lines 708 and 803 with explicit assertions that the
insufficient-quorum tests return OracleError::InsufficientOracleSources (7009).
Use update_rate() with captured error handling or try_update_rate() assertions,
ensuring both sites validate the expected contract error rather than accepting
unrelated panics.
In `@docs/ERROR_CODES.md`:
- Line 192: Update the ReserveTrackerError code assignments to give every
variant a unique, non-overlapping code, including AttestationNotFound,
InvalidMerkleProof, InvalidCustodian, AttestationExpired, NonPositiveAmount, and
InconsistentReserve. Then regenerate docs/ERROR_CODES.md so the catalog reflects
all corrected variants and codes, including replacing the incorrect
DuplicateCurrency entry.
---
Outside diff comments:
In `@acbu_oracle/src/lib.rs`:
- Around line 434-455: Fix update_rate by removing the obsolete initial
min-signatures/quorum block and ensuring the remaining multi-source quorum check
is properly closed before the following pub fn declarations. Preserve the
intended condition that only source counts greater than one and below the
required threshold panic with InsufficientOracleSources.
In `@acbu_reserve_tracker/src/lib.rs`:
- Around line 24-31: The ReserveTrackerError definitions contain duplicate
variant names and codes, and the error catalog is incomplete. In
acbu_reserve_tracker/src/lib.rs lines 24-31, consolidate the duplicate variants
while preserving one unique definition per error and assign non-conflicting
codes; then regenerate docs/ERROR_CODES.md line 192 to document every remaining
reserve-tracker variant with its finalized code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eba746a-8de7-4015-a432-f0622d499ddd
📒 Files selected for processing (5)
acbu_oracle/src/lib.rsacbu_oracle/tests/test.rsacbu_reserve_tracker/src/lib.rsacbu_reserve_tracker/tests/test.rsdocs/ERROR_CODES.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #333
fix #333 : oracle validator quorum check + reserve tracker duplicate currency guard
update_rate previously allowed 1 or 2 source feeds through as long as some sources were provided, meaning a single
potentially-malicious validator could set the median with no meaningful aggregation. The check now enforces a minimum of
max(min_signatures, MIN_ORACLE_SOURCE_FEEDS=3) sources whenever sources are provided. Passing zero sources (the existing bypass path
for direct rate submission) is unchanged.
Before:
if sources.len() > 0 && sources.len() < MIN_ORACLE_SOURCE_FEEDS {
env.panic_with_error(OracleError::InsufficientOracleSources);
}
After:
if sources.len() > 0 {
let required = min_sigs.max(MIN_ORACLE_SOURCE_FEEDS);
if sources.len() < required {
env.panic_with_error(OracleError::InsufficientOracleSources);
}
}
Issue #333 — Reserve tracker add_currency allows duplicates
add_currency pushed to a Vec without checking for duplicates. A duplicated currency entry causes iteration-based reserve lookups to
count the same reserve twice, inflating the reported total. The function now iterates the stored list before pushing and panics with
DuplicateCurrency (#8008) if the currency is already present.
Changes
MIN_ORACLE_SOURCE_FEEDS enforces the larger value
both last_verify_call and currencies
Notes
This branch is based on current dev with no merge conflicts. The old branch fix/319-333-oracle-quorum-reserve-duplicate-currency had a
DataKey conflict (both dev and that branch added a field simultaneously) and an incorrect quorum check that removed the > 0 guard, which would have broken zero-source submissions.
Summary by CodeRabbit
Bug Fixes
Tests