Refactor constructor to use parameter structs #40
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Djed.sol(2 hunks)
🔇 Additional comments (3)
src/Djed.sol (3)
40-61: LGTM! Clean parameter organization.The struct definitions effectively group related constructor parameters, improving code readability and maintainability. This refactoring reduces the constructor's parameter count from 12+ individual parameters to 3 well-organized structs.
63-69: Constructor signature refactored correctly.The updated constructor signature properly accepts the parameter structs. Using
memoryfor struct parameters is appropriate in a constructor context.
113-117: Excellent safety improvement for division-by-zero protection.The explicit liabilities check prevents division by zero when no stable coins have been minted yet. This makes the
ratio()function safe and predictable during early contract states. Reverting with a clear error message is the appropriate behavior when the reserve ratio is undefined.
| stableCoin = new Coin(coinParams.stableName, coinParams.stableSymbol); | ||
| reserveCoin = new Coin(coinParams.reserveName, coinParams.reserveSymbol); | ||
| scDecimalScalingFactor = 10**stableCoin.decimals(); | ||
| rcDecimalScalingFactor = 10**reserveCoin.decimals(); | ||
| scalingFactor = _scalingFactor; | ||
|
|
||
| treasury = _treasury; | ||
| initialTreasuryFee = _initialTreasuryFee; | ||
| treasuryRevenueTarget = _treasuryRevenueTarget; | ||
| treasury = treasuryParams.treasury; | ||
| initialTreasuryFee = treasuryParams.initialTreasuryFee; | ||
| treasuryRevenueTarget = treasuryParams.treasuryRevenueTarget; | ||
|
|
||
| reserveRatioMin = _reserveRatioMin; | ||
| reserveRatioMax = _reserveRatioMax; | ||
| fee = _fee; | ||
| thresholdSupplySC = _thresholdSupplySC; | ||
| rcMinPrice = _rcMinPrice; | ||
| rcInitialPrice = _rcInitialPrice; | ||
| txLimit = _txLimit; | ||
| reserveRatioMin = djedParams.reserveRatioMin; | ||
| reserveRatioMax = djedParams.reserveRatioMax; | ||
| fee = djedParams.fee; | ||
| thresholdSupplySC = djedParams.thresholdSupplySC; | ||
| rcMinPrice = djedParams.rcMinPrice; | ||
| rcInitialPrice = djedParams.rcInitialPrice; | ||
| txLimit = djedParams.txLimit; |
There was a problem hiding this comment.
Add comprehensive parameter validation before assignments.
The constructor correctly assigns parameters from the structs (fixing issue #18), but lacks validation of critical values before assigning them to immutable variables. Since these cannot be changed post-deployment, invalid parameters would permanently break the protocol.
Required validations:
treasuryParams.treasury != address(0)- prevents failed fee transferstreasuryParams.treasuryRevenueTarget > 0- prevents division by zero intreasuryFee()(line 210)_scalingFactor > 0- prevents division by zero in multiple functionsdjedParams.reserveRatioMin > 0 && djedParams.reserveRatioMin <= djedParams.reserveRatioMax- ensures valid ratio boundsdjedParams.reserveRatioMax > 0- ensures valid maximum ratiodjedParams.fee <= _scalingFactor- prevents fees exceeding 100%djedParams.rcInitialPrice > 0 && djedParams.rcMinPrice > 0- ensures valid reserve coin pricingdjedParams.txLimit > 0- ensures valid transaction limit
Apply this diff to add validation:
) payable {
+ require(treasuryParams.treasury != address(0), "Invalid treasury address");
+ require(treasuryParams.treasuryRevenueTarget > 0, "Treasury revenue target must be positive");
+ require(_scalingFactor > 0, "Scaling factor must be positive");
+ require(djedParams.reserveRatioMin > 0, "Min reserve ratio must be positive");
+ require(djedParams.reserveRatioMax > 0, "Max reserve ratio must be positive");
+ require(djedParams.reserveRatioMin <= djedParams.reserveRatioMax, "Min ratio must be <= max ratio");
+ require(djedParams.fee <= _scalingFactor, "Fee cannot exceed 100%");
+ require(djedParams.rcInitialPrice > 0, "RC initial price must be positive");
+ require(djedParams.rcMinPrice > 0, "RC min price must be positive");
+ require(djedParams.txLimit > 0, "Transaction limit must be positive");
+
stableCoin = new Coin(coinParams.stableName, coinParams.stableSymbol);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stableCoin = new Coin(coinParams.stableName, coinParams.stableSymbol); | |
| reserveCoin = new Coin(coinParams.reserveName, coinParams.reserveSymbol); | |
| scDecimalScalingFactor = 10**stableCoin.decimals(); | |
| rcDecimalScalingFactor = 10**reserveCoin.decimals(); | |
| scalingFactor = _scalingFactor; | |
| treasury = _treasury; | |
| initialTreasuryFee = _initialTreasuryFee; | |
| treasuryRevenueTarget = _treasuryRevenueTarget; | |
| treasury = treasuryParams.treasury; | |
| initialTreasuryFee = treasuryParams.initialTreasuryFee; | |
| treasuryRevenueTarget = treasuryParams.treasuryRevenueTarget; | |
| reserveRatioMin = _reserveRatioMin; | |
| reserveRatioMax = _reserveRatioMax; | |
| fee = _fee; | |
| thresholdSupplySC = _thresholdSupplySC; | |
| rcMinPrice = _rcMinPrice; | |
| rcInitialPrice = _rcInitialPrice; | |
| txLimit = _txLimit; | |
| reserveRatioMin = djedParams.reserveRatioMin; | |
| reserveRatioMax = djedParams.reserveRatioMax; | |
| fee = djedParams.fee; | |
| thresholdSupplySC = djedParams.thresholdSupplySC; | |
| rcMinPrice = djedParams.rcMinPrice; | |
| rcInitialPrice = djedParams.rcInitialPrice; | |
| txLimit = djedParams.txLimit; | |
| require(treasuryParams.treasury != address(0), "Invalid treasury address"); | |
| require(treasuryParams.treasuryRevenueTarget > 0, "Treasury revenue target must be positive"); | |
| require(_scalingFactor > 0, "Scaling factor must be positive"); | |
| require(djedParams.reserveRatioMin > 0, "Min reserve ratio must be positive"); | |
| require(djedParams.reserveRatioMax > 0, "Max reserve ratio must be positive"); | |
| require(djedParams.reserveRatioMin <= djedParams.reserveRatioMax, "Min ratio must be <= max ratio"); | |
| require(djedParams.fee <= _scalingFactor, "Fee cannot exceed 100%"); | |
| require(djedParams.rcInitialPrice > 0, "RC initial price must be positive"); | |
| require(djedParams.rcMinPrice > 0, "RC min price must be positive"); | |
| require(djedParams.txLimit > 0, "Transaction limit must be positive"); | |
| stableCoin = new Coin(coinParams.stableName, coinParams.stableSymbol); | |
| reserveCoin = new Coin(coinParams.reserveName, coinParams.reserveSymbol); | |
| scDecimalScalingFactor = 10**stableCoin.decimals(); | |
| rcDecimalScalingFactor = 10**reserveCoin.decimals(); | |
| scalingFactor = _scalingFactor; | |
| treasury = treasuryParams.treasury; | |
| initialTreasuryFee = treasuryParams.initialTreasuryFee; | |
| treasuryRevenueTarget = treasuryParams.treasuryRevenueTarget; | |
| reserveRatioMin = djedParams.reserveRatioMin; | |
| reserveRatioMax = djedParams.reserveRatioMax; | |
| fee = djedParams.fee; | |
| thresholdSupplySC = djedParams.thresholdSupplySC; | |
| rcMinPrice = djedParams.rcMinPrice; | |
| rcInitialPrice = djedParams.rcInitialPrice; | |
| txLimit = djedParams.txLimit; |
🤖 Prompt for AI Agents
In src/Djed.sol around lines 70 to 86, the constructor assigns many
immutable/config values without validating them; add require checks before the
assignments to prevent invalid permanent state. Specifically:
require(treasuryParams.treasury != address(0));
require(treasuryParams.treasuryRevenueTarget > 0); require(_scalingFactor > 0);
require(djedParams.reserveRatioMin > 0 && djedParams.reserveRatioMin <=
djedParams.reserveRatioMax); require(djedParams.reserveRatioMax > 0);
require(djedParams.fee <= _scalingFactor); require(djedParams.rcInitialPrice > 0
&& djedParams.rcMinPrice > 0); require(djedParams.txLimit > 0); place these
checks at the top of the constructor (before assigning immutables) with clear
revert messages so invalid inputs are rejected at deployment.
1 participant
This resolves Issue #18 by fixing two critical problems in the Djed contract.
Constructor parameter assignment
Corrected the constructor to properly initialize Djed parameters (reserveRatioMin, reserveRatioMax, fee, thresholdSupplySC, rcMinPrice, rcInitialPrice, txLimit) from the djedParams struct instead of incorrectly referencing the struct type.
This ensures all protocol parameters are set as intended at deployment time.
Reserve ratio calculation safety
Updated the ratio() function to explicitly handle the case where liabilities are zero.
Added a guard check to prevent division by zero and make the function behavior safe and predictable during early or edge states.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.