Fix oracle rate initialization - return RateNotInitialized error befo…#442
Conversation
…re first submission - Add RateNotInitialized error variant (7023) to distinguish between unregistered currencies and no submissions yet - Modify get_rate/get_rate_with_timestamp to return RateNotInitialized instead of RateNotFound when no rate exists - Modify get_acbu_usd_rate/get_acbu_usd_rate_with_timestamp to panic with RateNotInitialized instead of returning DECIMALS - Add test to verify all rate getters fail before first submission - Prevents division by zero or 0-rate mints/burns before first epoch
- Add FeeRateUpdated event type with old/new fee rates and timestamp - Emit events in set_fee_rate functions for both contracts - Emit events in set_fee_single (minting) and set_fee_single_redeem (burning) - Backend listeners can now track fee changes without polling
Backup files should not be committed to version control as they can be confused with actual implementation during code review or refactoring.
- Add SelfEscrow error variant (3017) to EscrowError enum - Add validation check in create function to reject when payer == payee - Add test to verify self-escrow is rejected with SelfEscrow error - Prevents gas waste from circular transfers and artificial volume inflation
|
@wokedi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdded typed fee update events to burning and minting, rejected escrow creation when payer and payee match, and changed oracle reads to error until rates are initialized. Added tests for the escrow and oracle behaviors. ChangesFee rate update events
Self-escrow validation
Oracle initialization checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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 (2)
547-565: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winRestore the missing basis-points scaling in
get_acbu_usd_rate_with_timestamp.
weighted_sumis already normalized byBASIS_POINTSinside the loop, but Line 565 divides it bytotal_weightwithout scaling back up. That makes this getter return a price ~10,000× too small versusget_acbu_usd_rate, and downstream callers likeacbu_minting::MintingContract::mint_from_usdcwill mint against a severely distorted ACBU price.Suggested fix
- let rate = weighted_sum / total_weight; + let rate = weighted_sum + .checked_mul(BASIS_POINTS) + .and_then(|v| v.checked_div(total_weight)) + .unwrap_or_else(|| env.panic_with_error(OracleError::InvalidRate));🤖 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 547 - 565, In get_acbu_usd_rate_with_timestamp, the weighted average is missing the BASIS_POINTS scale correction because weighted_sum is already divided inside the loop but the final rate is computed only by total_weight. Update the final rate calculation in this function so it restores the basis-points scaling used by get_acbu_usd_rate, keeping the returned ACBU USD price consistent for callers like MintingContract::mint_from_usdc.
547-559: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a fully initialized basket before returning an ACBU/USD rate.
These loops still skip currencies with no stored rate and only fail when
total_weight == 0. If just one non-zero-weight currency has been submitted, both ACBU/USD getters now succeed with a renormalized partial-basket price instead ofRateNotInitialized, so USDC mint/burn paths can still proceed before the basket is fully initialized.Also applies to: 595-607
🤖 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 547 - 559, The ACBU/USD rate calculation in the relevant getter loops is still allowing partial baskets to pass by skipping currencies with missing stored rates and only checking total_weight, so update the rate assembly logic in the ACBU/USD getter path to require every basket currency to have an initialized rate before returning a price. In the loop inside the ACBU/USD rate function(s), use the existing basket_weights and get_rate_internal flow to detect any missing rate_data and immediately return RateNotInitialized instead of renormalizing a partial basket; ensure the same fix is applied to both getter implementations referenced by the diff.
🤖 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/tests/test.rs`:
- Around line 686-700: The pre-submission oracle getter checks in test.rs only
assert that an error occurred, so they won’t catch regressions in the error
contract. Update the assertions around client.try_get_rate,
client.try_get_rate_with_timestamp, client.try_get_acbu_usd_rate, and
client.try_get_acbu_usd_rate_with_timestamp to verify the specific
RateNotInitialized error instead of only using is_err(). Use the existing client
and ngn setup in this test block to match the exact error variant returned by
each try_* method.
---
Outside diff comments:
In `@acbu_oracle/src/lib.rs`:
- Around line 547-565: In get_acbu_usd_rate_with_timestamp, the weighted average
is missing the BASIS_POINTS scale correction because weighted_sum is already
divided inside the loop but the final rate is computed only by total_weight.
Update the final rate calculation in this function so it restores the
basis-points scaling used by get_acbu_usd_rate, keeping the returned ACBU USD
price consistent for callers like MintingContract::mint_from_usdc.
- Around line 547-559: The ACBU/USD rate calculation in the relevant getter
loops is still allowing partial baskets to pass by skipping currencies with
missing stored rates and only checking total_weight, so update the rate assembly
logic in the ACBU/USD getter path to require every basket currency to have an
initialized rate before returning a price. In the loop inside the ACBU/USD rate
function(s), use the existing basket_weights and get_rate_internal flow to
detect any missing rate_data and immediately return RateNotInitialized instead
of renormalizing a partial basket; ensure the same fix is applied to both getter
implementations referenced by the diff.
🪄 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
Run ID: cf8bb573-7074-4841-b1f4-9bab2837f653
📒 Files selected for processing (7)
acbu_burning/src/lib.rsacbu_escrow/src/lib.rsacbu_escrow/tests/test.rsacbu_lending_pool/src/lib.rs.bakacbu_minting/src/lib.rsacbu_oracle/src/lib.rsacbu_oracle/tests/test.rs
| // get_rate should fail before first submission | ||
| let result = client.try_get_rate(&ngn); | ||
| assert!(result.is_err(), "get_rate should fail before first submission"); | ||
|
|
||
| // get_rate_with_timestamp should fail before first submission | ||
| let result = client.try_get_rate_with_timestamp(&ngn); | ||
| assert!(result.is_err(), "get_rate_with_timestamp should fail before first submission"); | ||
|
|
||
| // get_acbu_usd_rate should fail before first submission | ||
| let result = client.try_get_acbu_usd_rate(); | ||
| assert!(result.is_err(), "get_acbu_usd_rate should fail before first submission"); | ||
|
|
||
| // get_acbu_usd_rate_with_timestamp should fail before first submission | ||
| let result = client.try_get_acbu_usd_rate_with_timestamp(); | ||
| assert!(result.is_err(), "get_acbu_usd_rate_with_timestamp should fail before first submission"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the specific RateNotInitialized error here.
These checks pass on any failure, so they won't catch regressions where the getters start erroring for the wrong reason. Since this PR is changing the oracle error contract, the test should match RateNotInitialized for each try_* call instead of only checking is_err().
🤖 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/tests/test.rs` around lines 686 - 700, The pre-submission oracle
getter checks in test.rs only assert that an error occurred, so they won’t catch
regressions in the error contract. Update the assertions around
client.try_get_rate, client.try_get_rate_with_timestamp,
client.try_get_acbu_usd_rate, and client.try_get_acbu_usd_rate_with_timestamp to
verify the specific RateNotInitialized error instead of only using is_err(). Use
the existing client and ngn setup in this test block to match the exact error
variant returned by each try_* method.
2 participants
closes #331
closes #340
added FeeRateUpdated events to both minting and burning contracts:
Changes to acbu_minting/src/lib.rs:
Added FeeRateUpdatedEvent struct with old_fee_rate_bps, new_fee_rate_bps, and timestamp
Modified set_fee_rate to emit event with topic "fee_upd"
Modified set_fee_single to emit event with topic "fee_sgl"
Changes to acbu_burning/src/lib.rs:
Added FeeRateUpdatedEvent struct with same fields
Modified set_fee_rate to emit event with topic "fee_upd"
Modified set_fee_single_redeem to emit event with topic "fee_sgl"
Backend listeners can now track fee rate changes in real-time without polling. The events include both old and new values along with the timestamp for audit trails.
closes #345
Removed the backup file acbu_lending_pool/src/lib.rs.bak from the repository. Backup files should not be committed to version control as they can cause confusion during code review or refactoring.
closes #335
Fixed the self-escrow issue in the escrow contract:
Changes to acbu_escrow/src/lib.rs:
Added SelfEscrow error variant (3017) to EscrowError enum
Added validation check in create function to reject when payer == payee
Added error message "payee cannot be the same as payer" in Display implementation
Summary by CodeRabbit