enhanced-events: Enhance Protocol Observability via Event Updates#51
Open
aniket866 wants to merge 3 commits into
Open
enhanced-events: Enhance Protocol Observability via Event Updates#51aniket866 wants to merge 3 commits into
aniket866 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughEvent signatures for buy/sell operations in the Djed contracts were extended to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🤖 Fix all issues with AI agents
In `@src/Djed.sol`:
- Around line 103-106: The emitted post-trade ratio uses a stale price: keep scP
= scPrice(msg.value) as the pre-trade/currentPrice but compute postRatio using
scPrice(0) (the post-trade reserve-based price used by ratio() and
isRatioAboveMin) so the event reflects the same post-trade semantics;
specifically in buyStableCoins compute postRatio with scPrice(0) when combining
newReserveBalance and stableCoin.totalSupply() (after mint) and update the same
pattern in buyReserveCoins (use scPrice(0) for postRatio while retaining
scP/scPrice(msg.value) as the currentPrice variable).
- Around line 133-136: The BoughtReserveCoins/SoldReserveCoins/SoldBothCoins
events are emitting scP as the "currentPrice" which misrepresents the actual
per-RC execution price; update the emit calls so BoughtReserveCoins emits rcBP
(the buy-RC per-unit price computed at line with rcBP), SoldReserveCoins emits
rcTargetPrice(scP, 0) (the sell-RC per-unit price), and for SoldBothCoins
include both scP and the RC per-unit price (rcTargetPrice(scP,0)) or rename the
event field to scPrice if you prefer keeping scP; adjust the event signature(s)
and all emit sites (BoughtReserveCoins, SoldReserveCoins, SoldBothCoins) to
match the chosen approach so event consumers receive the actual per-unit
price(s).
In `@src/DjedShu.sol`:
- Around line 108-111: postRatio is computed using different price variants per
function (scMaxPrice(msg.value), scMinPrice(0), scMinPrice(msg.value),
scMaxPrice(0), etc.), making the emitted postRatio ambiguous and sometimes using
a pre-trade/stale price; update all five functions (buyStableCoins,
sellStableCoins, buyReserveCoins, sellReserveCoins, sellBothCoins) to compute
postRatio using a single consistent price definition — prefer scMaxPrice(0) to
match ratioMax() — and always call the price helper with offset 0 (no msg.value)
so post-trade price is consistent; factor the ratio computation into a shared
helper (e.g., computePostRatio or similar) that takes newReserveBalance and
stableCoin.totalSupply() and returns postRatio to avoid duplication and ensure
all events emit the same, unambiguous price.
- Around line 177-178: The require check is using L(scMinPrice(0)) but the
intended stronger bound compares R(0)/L(scMaxPrice(0)) against preR/preL; update
the expression in the require inside sellBoth to use L(scMaxPrice(0)) instead of
L(scMinPrice(0)) so it reads require(R(0) * preL >= preR * L(scMaxPrice(0)),
"sellBoth: ratio decreased"); alternatively, if you intentionally want the
weaker bound keep the code and change the comment to document that the check
uses scMinPrice and thus enforces the weaker inequality; ensure references to
R(0), preR, preL, L(...), scMinPrice(0) and scMaxPrice(0) are consistent.
🧹 Nitpick comments (1)
src/Djed.sol (1)
103-106: Extract postRatio computation into a helper to eliminate duplication.The identical three-line block (compute
newReserveBalance, computepostRatiowith the zero-supply guard, emit) is repeated across all five trading functions. A private helper would consolidate the logic, reduce bytecode size, and make future changes (e.g., fixing the price used for ratio) a single-point edit.Proposed helper
+ function _computePostRatio(uint256 _scPrice) internal view returns (uint256 newReserveBalance, uint256 postRatio) { + newReserveBalance = R(0); + uint256 totalSC = stableCoin.totalSupply(); + postRatio = (totalSC == 0 || _scPrice == 0) + ? 0 + : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (totalSC * _scPrice); + }Then each emit site becomes:
(uint256 newReserveBalance, uint256 postRatio) = _computePostRatio(scPrice(0)); emit BoughtStableCoins(msg.sender, receiver, amountSC, msg.value, postRatio, scP, newReserveBalance);Also applies to: 118-121, 133-136, 149-152, 168-173
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/DjedShu.sol (1)
252-255: RedundantscMaxPrice(0)calls — consider returningscPfrom the helper.
computePostRatiocomputesscMaxPrice(0)internally, but every call site also passesscMaxPrice(0)separately as thecurrentPriceargument in the emit. This doubles the oracle read cost per transaction. InsellBothCoins,scMaxPrice(0)is called up to four times.Returning the price from the helper eliminates the duplication:
♻️ Proposed refactor
- function computePostRatio(uint256 newReserveBalance) internal view returns (uint256) { - uint256 scP = scMaxPrice(0); - return (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP); - } + function computePostRatio(uint256 newReserveBalance) internal view returns (uint256 postRatio, uint256 scP) { + scP = scMaxPrice(0); + postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP); + }Then at each call site (e.g.
buyStableCoins):uint256 newReserveBalance = R(0); - uint256 postRatio = computePostRatio(newReserveBalance); - emit BoughtStableCoins(msg.sender, receiver, amountSC, msg.value, postRatio, scMaxPrice(0), newReserveBalance); + (uint256 postRatio, uint256 currentPrice) = computePostRatio(newReserveBalance); + emit BoughtStableCoins(msg.sender, receiver, amountSC, msg.value, postRatio, currentPrice, newReserveBalance);
aniket866
force-pushed
the
enhanced-events
branch
from
c367d7d to
914e886
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Djed.sol (2)
134-136:⚠️ Potential issue | 🟠 MajorUse the post-trade SC price for
buyReserveCoins.postRatio.
postRatiois still dividing by the pre-tradescPfrom Line 125. After minting RC and retaining fees, the reserve state has changed, so this event can emit a stale ratio even thoughcurrentPriceshould stayrcBP.Proposed fix
uint256 newReserveBalance = R(0); - uint256 postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP); + uint256 scPPost = scPrice(0); + uint256 postRatio = (stableCoin.totalSupply() == 0 || scPPost == 0) + ? 0 + : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scPPost); emit BoughtReserveCoins(msg.sender, receiver, amountRC, msg.value, postRatio, rcBP, newReserveBalance);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Djed.sol` around lines 134 - 136, The emitted postRatio in BoughtReserveCoins is using the pre-trade scP and can be stale; after minting RC and applying fees you must recompute the stablecoin price/parameters from the updated state and use those to compute postRatio. Update the logic around newReserveBalance/postRatio (referencing newReserveBalance, postRatio, stableCoin.totalSupply(), scP, currentPrice, rcBP and the BoughtReserveCoins event) so that you first apply the mint/fee state changes, derive the post-trade sc price (e.g., use currentPrice or recompute scP from the updated totalSupply/reserve), then compute postRatio using the post-trade scP and updated stableCoin.totalSupply() before emitting the event.
38-38:⚠️ Potential issue | 🟠 Major
SoldBothCoins.currentPriceis still ambiguous.
sellBothCoinsexecutes with bothscPandrcP(Lines 159-160), but the event schema only exposes onecurrentPriceslot and the emit picksrcP. That still falls short of the PR objective to expose the exact price used for the trade from logs alone.Also applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Djed.sol` at line 38, The SoldBothCoins event only exposes a single ambiguous currentPrice while sellBothCoins computes both scP and rcP; update the event schema to include distinct price fields (e.g., currentSCPrice and currentRCPrice or scPrice and rcPrice) and change the emit in sellBothCoins to pass both scP and rcP accordingly; also apply the same change to the analogous event/emit pair referenced around lines 172-174 so logs unambiguously show the exact prices used for the trade.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Djed.sol`:
- Around line 119-121: The emitted postRatio uses the pre-trade price variable
scP but is logged after reserve/supply have been updated; update the code paths
in the sell flow (the blocks that compute newReserveBalance = R(0) and then
compute postRatio) to recompute scP using the post-trade state before
calculating postRatio (i.e., derive the new SC price from the updated reserve
and stableCoin.totalSupply(), or recalculate scP via the same function that
produces the on-chain price after modifying reserves/supply) and then use that
recomputed scP in the postRatio formula (still using scalingFactor,
scDecimalScalingFactor and stableCoin.totalSupply()). Ensure all three sell-side
emit points (those using newReserveBalance, R(0), and dividing by scP) are
changed to use the post-trade scP.
---
Duplicate comments:
In `@src/Djed.sol`:
- Around line 134-136: The emitted postRatio in BoughtReserveCoins is using the
pre-trade scP and can be stale; after minting RC and applying fees you must
recompute the stablecoin price/parameters from the updated state and use those
to compute postRatio. Update the logic around newReserveBalance/postRatio
(referencing newReserveBalance, postRatio, stableCoin.totalSupply(), scP,
currentPrice, rcBP and the BoughtReserveCoins event) so that you first apply the
mint/fee state changes, derive the post-trade sc price (e.g., use currentPrice
or recompute scP from the updated totalSupply/reserve), then compute postRatio
using the post-trade scP and updated stableCoin.totalSupply() before emitting
the event.
- Line 38: The SoldBothCoins event only exposes a single ambiguous currentPrice
while sellBothCoins computes both scP and rcP; update the event schema to
include distinct price fields (e.g., currentSCPrice and currentRCPrice or
scPrice and rcPrice) and change the emit in sellBothCoins to pass both scP and
rcP accordingly; also apply the same change to the analogous event/emit pair
referenced around lines 172-174 so logs unambiguously show the exact prices used
for the trade.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d48770-2abc-4f48-857e-6e9606920d2d
📒 Files selected for processing (2)
src/Djed.solsrc/DjedShu.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- src/DjedShu.sol
Comment on lines
+119
to
+121
| uint256 newReserveBalance = R(0); | ||
| uint256 postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP); | ||
| emit SoldStableCoins(msg.sender, receiver, amountSC, amountBC, postRatio, scP, newReserveBalance); |
There was a problem hiding this comment.
Compute sell-side postRatio with the post-trade SC price.
These three paths emit postRatio after reserve and/or SC supply changed, but they still divide by the pre-trade scP from Lines 112, 141, and 159. In reserve-constrained states, the logged ratio will drift from the actual post-trade ratio the protocol ended with.
Proposed fix
uint256 newReserveBalance = R(0);
- uint256 postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP);
+ uint256 scPPost = scPrice(0);
+ uint256 postRatio = (stableCoin.totalSupply() == 0 || scPPost == 0)
+ ? 0
+ : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scPPost);
emit SoldStableCoins(msg.sender, receiver, amountSC, amountBC, postRatio, scP, newReserveBalance); uint256 newReserveBalance = R(0);
- uint256 postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP);
+ uint256 scPPost = scPrice(0);
+ uint256 postRatio = (stableCoin.totalSupply() == 0 || scPPost == 0)
+ ? 0
+ : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scPPost);
emit SoldReserveCoins(msg.sender, receiver, amountRC, amountBC, postRatio, rcP, newReserveBalance); uint256 newReserveBalance = R(0);
- uint256 postRatio = (stableCoin.totalSupply() == 0 || scP == 0) ? 0 : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scP);
+ uint256 scPPost = scPrice(0);
+ uint256 postRatio = (stableCoin.totalSupply() == 0 || scPPost == 0)
+ ? 0
+ : (newReserveBalance * scalingFactor * scDecimalScalingFactor) / (stableCoin.totalSupply() * scPPost);
emit SoldBothCoins(msg.sender, receiver, amountSC, amountRC, amountBC, postRatio, rcP, newReserveBalance);Also applies to: 151-153, 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Djed.sol` around lines 119 - 121, The emitted postRatio uses the
pre-trade price variable scP but is logged after reserve/supply have been
updated; update the code paths in the sell flow (the blocks that compute
newReserveBalance = R(0) and then compute postRatio) to recompute scP using the
post-trade state before calculating postRatio (i.e., derive the new SC price
from the updated reserve and stableCoin.totalSupply(), or recalculate scP via
the same function that produces the on-chain price after modifying
reserves/supply) and then use that recomputed scP in the postRatio formula
(still using scalingFactor, scDecimalScalingFactor and
stableCoin.totalSupply()). Ensure all three sell-side emit points (those using
newReserveBalance, R(0), and dividing by scP) are changed to use the post-trade
scP.
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.
Addressed Issues:
Closes #50
Summary of Changes
Added three new data points to all core trading events (BoughtStableCoins, SoldStableCoins, BoughtReserveCoins, SoldReserveCoins, and SoldBothCoins) in both Djed.sol and DjedShu.sol:
postRatio: The reserve ratio immediately after the transaction.
currentPrice: The exact price used for the trade.
newReserveBalance: The total amount of Base Coin (e.g., ETH/ADA) held in the contract after the trade.
Why?
Real-Time Monitoring: Currently, events only log basic amounts and addresses. Adding these fields allows dashboards to show the protocol's health (ratio and reserves) instantly without making extra, delayed queries to the blockchain.
Bot & Arbitrage Efficiency: Arbitrage bots and liquidators need to know the exact state of the protocol to function. By including the price and ratio in the event log, bots can react faster to market changes, which helps maintain the stablecoin’s peg.
Reduced Complexity: External applications no longer need to manually calculate the "post-trade" state; they can simply read it directly from the transaction logs.
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
@Zahnentferner Please review
Summary by CodeRabbit