feat: introduce dynamic mining pass price DEV-1084 - #65
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds dynamic mining-pass pricing: on‑chain APIs, storage and initializers for tier/base fees and fee floor; Hardhat tasks and scripts to compute/apply dynamic fees using VAPE price/subgraph data; multiple analysis scripts and migration tooling; unit and fork tests; and updated deployment artifacts and tool configs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 12
🤖 Fix all issues with AI agents
In @.vscode/settings.json:
- Around line 1-5: Remove the committed VSCode workspace settings and ignore
them going forward: delete the settings file from the PR (unstage/remove it from
the repository commit, e.g., git rm --cached) and add the .vscode/ directory to
.gitignore so IDE settings are not tracked; specifically remove
".vscode/settings.json" from the repo and add a ".vscode/" entry to .gitignore,
then commit the updated .gitignore.
In `@scripts/simulation/analyzeUtilization.ts`:
- Around line 67-69: The pctNotMaxed calculation can produce NaN when
totalNotProfit is 0; update the code that computes pctNotMaxed (currently using
totalNotProfit and notProfitableNotMaxed) to guard against division by zero by
computing pctNotMaxed only if totalNotProfit > 0 (e.g., use a conditional or
ternary) and otherwise set a safe formatted value like "0.0" (so toFixed is not
called on NaN), then use that safe value in the console.log.
- Around line 38-40: The current calculation for pctOfLimit divides deposited by
limit and can produce Infinity/NaN when tierLimits[limitStr] is undefined (limit
=== 0); update the logic around the variables limit, limitStr, tierLimits,
deposited and pctOfLimit to guard against a zero/undefined limit: check if limit
is a positive number before doing (deposited / limit) and if not, set pctOfLimit
to a safe default string (e.g., "0.0" or "N/A") or handle it explicitly so no
division occurs.
In `@scripts/simulation/exportSeasonAnalysis.ts`:
- Around line 230-242: Guard all divisions in the summary by checking
denominators before computing ratios: when building rows with totalOriginalFees,
totalDynamicFees, usersWithPaidPass, and when computing ROI/percentages for
originalProfitable/dynamicProfitable use conditional logic to avoid dividing by
zero (e.g., if totalOriginalFees or totalDynamicFees is 0, output "0.00x" or
"N/A" for ROI; if usersWithPaidPass is 0, output "0 / 0 (0.0%)" or "N/A" for
percentages and improvement), updating the code that pushes rows (references:
rows.push lines that use totalOriginalFees, totalDynamicFees, totalVAPEValue,
usersWithPaidPass, originalProfitable, dynamicProfitable) and apply the same
guards to the console.log statements around the later summary output (console
logs referenced at ~277–279) so no division-by-zero produces NaN/Infinity.
In `@scripts/simulation/miningPassPricing.ts`:
- Around line 188-196: The minimum-price clamp is forcing tier 0 (free) passes
to cost $0.01; update the logic in miningPassPricing.ts so the Math.max(...,
0.01) floor is only applied for paid tiers. Specifically, where dynamicPriceUSDC
is set (and in calculateSimpleDynamicPrice), change the assignment so that if
tier === 0 you keep dynamicPriceUSDC (or originalPrice) as 0, otherwise apply
the Math.max(..., 0.01) clamp; adjust any related fields (e.g.,
dynamicPriceUSDC, originalPriceUSDC) so tier 0 remains zero-priced and paid
tiers retain the $0.01 minimum.
In `@scripts/simulation/miningPassROIAnalysis.ts`:
- Around line 155-159: The calculateDynamicFee function applies a $0.01 minimum
to all tiers, which incorrectly charges tier 0 (original fee 0); update
calculateDynamicFee (and use ORIGINAL_TIER_FEES_USDC and REFERENCE_VAPE_PRICE)
to return 0 immediately if the originalFee is 0, otherwise compute originalFee *
(vapePrice / REFERENCE_VAPE_PRICE) and then apply Math.max(..., 0.01) so only
paid tiers get the $0.01 floor.
- Around line 393-398: Avoid division-by-zero when computing percentages and
improvement: guard uses of totalUsers and totalProfitableOrig before dividing
(the console.log lines that compute percentages with (totalProfitableOrig /
totalUsers) and (totalProfitableDyn / totalUsers) and the improvement
calculation using (totalProfitableDyn - totalProfitableOrig) /
totalProfitableOrig). If totalUsers is zero, print a safe placeholder (e.g.,
"N/A" or "0.0%") instead of performing the division; if totalProfitableOrig is
zero, skip or format the improvement as "N/A" (or show the absolute delta only)
rather than dividing by zero. Update the blocks that compute the percentage
strings and the improvement variable (references: totalProfitableOrig,
totalProfitableDyn, totalUsers, improvement) to perform these guards before
formatting and logging.
In `@scripts/simulation/unprofitableAnalysis.ts`:
- Around line 93-104: The final console.log divides grandTotalLoss by
unprofitableUsers.length which can be zero; guard that division by computing an
averageLoss variable like avgPerUser = unprofitableUsers.length ? grandTotalLoss
/ unprofitableUsers.length : 0 and use that in the TOTAL line formatting so you
never produce NaN; update the TOTAL console.log to use avgPerUser (formatted
toFixed(2) and padStart as before) and reference grandTotalLoss and
unprofitableUsers in the same scope.
- Around line 45-47: The calculation of utilization can divide by zero when
tierLimits[limitStr] is missing (limit becomes 0), causing Infinity; update the
logic around const limit = tierLimits[limitStr] || 0 and the utilization
calculation to guard against zero/NaN: compute deposited from depositedStr as
before, check if limit is a positive number (e.g., > 0) before doing (deposited
/ limit) * 100 and otherwise set utilization to 0 (or null/NaN per project
convention), mirroring the safe handling used in analyzeUtilization.ts and
ensure any downstream code can handle the fallback value.
In `@src/facets/DiamondManagerFacet.sol`:
- Around line 164-185: The emitted MiningPassFeesUpdated from
setMiningPassTierFee doesn't include which tier changed; update the contract to
emit a tier-aware event: either add a new event like
MiningPassTierFeeUpdated(uint256 indexed tier, uint256 fee) and emit it from
setMiningPassTierFee, or change the existing MiningPassFeesUpdated signature to
include an indexed tier and emit that with (tier, fee); ensure you update any
places that listen for MiningPassFeesUpdated and adjust tests to expect the new
event and payload, while keeping the storage write to
s.miningPassTierToFee[tier] as-is.
- Around line 187-195: The new floor set by setMiningPassFeeFloor can invalidate
existing tier fees in s.miningPassTierToFee, so modify setMiningPassFeeFloor to
either (A) iterate all existing mining pass tiers (the same index/collection
used by setMiningPassFees/setMiningPassTierFee) and revert if any stored fee <
floorBps, or (B) add a runtime clamp in the purchase path (where mining pass
price is read) to use max(storedFee, s.miningPassFeeFloorBps). Pick one
approach: for write-time validation, loop the stored tiers in
setMiningPassFeeFloor and revert with DiamondManagerFacet__Invalid_Input() if
any fee < floorBps; for runtime safety, change the purchase logic to compute
price = max(s.miningPassTierToFee[tier], s.miningPassFeeFloorBps) before
charging. Ensure you reference and update the same storage keys
(s.miningPassTierToFee, s.miningPassFeeFloorBps) and related functions
(setMiningPassFees, setMiningPassTierFee, and the purchase function)
accordingly.
In `@src/libraries/AppStorage.sol`:
- Around line 80-83: The two new storage fields baseMiningPassTierToFee and
miningPassFeeFloorBps were inserted mid-struct in AppStorage causing a storage
layout collision; move these declarations out of the MINING PASS/GENERAL region
and append them after the last existing storage variable isSeasonClaimed in the
AppStorage struct so they occupy new slots, update any nearby comments to
reflect their new position, and ensure no other fields are reordered or removed.
🧹 Nitpick comments (8)
package.json (1)
71-73:viemshould be adevDependency, not a productiondependency.
viemis only consumed by simulation/analysis scripts underscripts/simulation/. Placing it independenciesmeans it will be installed by consumers of this package. Move it todevDependencies.Proposed fix
- "dependencies": { - "viem": "2.45.1" - } + "devDependencies": { + ... + "viem": "2.45.1", + ... + }scripts/simulation/exportSeasonAnalysis.ts (1)
91-110: No HTTP status check before parsing the response.If the subgraph returns a non-200 response,
response.json()may throw or return unexpected data. Checkresponse.okbefore parsing.Proposed fix
const response = await fetch(SUBGRAPH_URL, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ query }), }) + if (!response.ok) { + console.log(`Subgraph returned ${response.status}, stopping pagination.`) + hasMore = false + continue + } const data = await response.json()scripts/simulation/miningPassPricing.ts (2)
5-5: Unused constantVAPE_TOKEN.
VAPE_TOKENis declared but never referenced in this file.
198-212: DefaultreferenceVAPEPrice = 1.0is misleading.The constant
REFERENCE_VAPE_PRICEis0.606, but the function defaults to1.0if the caller omits the parameter. This inconsistency could produce incorrect pricing if the function is later called without an explicit reference price.Proposed fix
function calculateSimpleDynamicPrice( tier: number, vapePrice: number, - referenceVAPEPrice: number = 1.0 + referenceVAPEPrice: number = REFERENCE_VAPE_PRICE ): { originalPrice: number; dynamicPrice: number; multiplier: number } {scripts/simulation/miningPassROIAnalysis.ts (1)
104-132:fetchUsersForSeasonis limited to 1000 users with no pagination.Unlike the paginated
fetchAllUsersForSeasoninexportSeasonAnalysis.ts, this function fetches at most 1000 users. If a season has more participants, the analysis will be based on incomplete data, silently skewing ROI metrics.Consider adding pagination or at least logging a warning when the result hits the limit.
Proposed minimal warning
const data = await response.json() - return data.data?.users?.items || [] + const items = data.data?.users?.items || [] + if (items.length === 1000) { + console.warn(`⚠️ Season ${seasonId}: hit 1000 user limit — results may be incomplete.`) + } + return itemssrc/facets/DiamondManagerFacet.sol (1)
456-499: Getters lack tier bounds validation, unlike their setter counterparts.
getMiningPassTierFee,getBaseMiningPassTierFee,getMiningPassTierFloorFee, andgetMiningPassTierDepositLimitall accept an unboundedtierparameter and silently return0(default storage) for out-of-range values. The setters enforcetier <= 10. Off-chain consumers may misinterpret a0return for tier 99 as valid data.Consider adding a
tier > 10revert (matchingsetMiningPassTierFee) or documenting the behavior explicitly.♻️ Example bounds check for getters
function getMiningPassTierFee(uint256 tier) external view returns (uint256) { + if (tier > 10) revert DiamondManagerFacet__InvalidTier(); return s.miningPassTierToFee[tier]; } function getBaseMiningPassTierFee(uint256 tier) external view returns (uint256) { + if (tier > 10) revert DiamondManagerFacet__InvalidTier(); return s.baseMiningPassTierToFee[tier]; } function getMiningPassTierFloorFee(uint256 tier) external view returns (uint256) { + if (tier > 10) revert DiamondManagerFacet__InvalidTier(); uint256 baseFee = s.baseMiningPassTierToFee[tier]; return (baseFee * s.miningPassFeeFloorBps) / TOTAL_SHARES; } function getMiningPassTierDepositLimit(uint256 tier) external view returns (uint256) { + if (tier > 10) revert DiamondManagerFacet__InvalidTier(); return s.miningPassTierToDepositLimit[tier]; }test/foundry/Facets/MiningPassDynamicPricing.t.sol (2)
98-120: Consider verifying event emissions in setter tests.The
setMiningPassFees,setMiningPassTierFee, andsetMiningPassFeeFloorfunctions all emit events, but none of the tests usevm.expectEmitto verify them. Adding event assertions (especially forMiningPassFeesUpdatedandMiningPassFeeFloorUpdated) would strengthen coverage and catch regressions in event parameters.
140-153: Floor enforcement test for bulk set relies on tier 1 reverting first — consider documenting or adding an isolated test.This test constructs fees at 24% across all tiers. The revert will trigger on tier 1 (the first non-zero tier in the loop). If the loop order or skip logic changes, the test may become misleading. The behavior is correct today, but it's worth noting this is an implicit ordering dependency.
| { | ||
| "python-envs.defaultEnvManager": "ms-python.python:conda", | ||
| "python-envs.defaultPackageManager": "ms-python.python:conda", | ||
| "python-envs.pythonProjects": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if .vscode is in .gitignore
if [ -f .gitignore ]; then
echo "=== .gitignore content (searching for .vscode) ==="
rg -n '\.vscode' .gitignore || echo "No .vscode found in .gitignore"
echo ""
echo "=== Full .gitignore content (first 50 lines) ==="
head -50 .gitignore
else
echo ".gitignore file not found"
fiRepository: VaporFi/liquid-mining
Length of output: 461
🏁 Script executed:
# Check if there are other .vscode files in the repo
echo "=== All .vscode files in the repository ==="
find . -path './.git' -prune -o -type f -path '*/.vscode/*' -print 2>/dev/null | head -20Repository: VaporFi/liquid-mining
Length of output: 132
🏁 Script executed:
# Check git diff to see what was actually added in this commit/PR
echo "=== Check if .vscode/settings.json is a new file ==="
git ls-files .vscode/ 2>/dev/null || echo "No .vscode files tracked in git"Repository: VaporFi/liquid-mining
Length of output: 139
🏁 Script executed:
# Check for Python files in the repository
echo "=== Python files in repository ==="
find . -path './.git' -prune -o -type f \( -name "*.py" \) -print 2>/dev/null | head -20Repository: VaporFi/liquid-mining
Length of output: 137
🏁 Script executed:
# Check the PR/recent git context to understand the scope
echo "=== Repository structure (top level) ==="
ls -la | grep -v '^d' | head -20
echo ""
echo "=== Directory structure ==="
find . -maxdepth 2 -type d ! -path '*/\.git*' ! -path '*/.vscode*' ! -path '*/node_modules*' | sort | head -30Repository: VaporFi/liquid-mining
Length of output: 1712
Add .vscode/ to .gitignore and remove .vscode/settings.json from this PR.
The repository already ignores IDE configurations (.idea is in .gitignore). VSCode workspace settings should follow the same pattern rather than being committed. With only one Python script in a Solidity-focused project, this appears to be personal developer configuration, not a project requirement.
🤖 Prompt for AI Agents
In @.vscode/settings.json around lines 1 - 5, Remove the committed VSCode
workspace settings and ignore them going forward: delete the settings file from
the PR (unstage/remove it from the repository commit, e.g., git rm --cached) and
add the .vscode/ directory to .gitignore so IDE settings are not tracked;
specifically remove ".vscode/settings.json" from the repo and add a ".vscode/"
entry to .gitignore, then commit the updated .gitignore.
| const limit = tierLimits[limitStr] || 0 | ||
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | ||
| const pctOfLimit = ((deposited / limit) * 100).toFixed(1) |
There was a problem hiding this comment.
Division by zero when limitStr doesn't match any key in tierLimits.
When tierLimits[limitStr] is undefined, limit becomes 0, and the division on line 40 produces Infinity/NaN. Guard against this.
Proposed fix
const limit = tierLimits[limitStr] || 0
+ if (limit === 0) continue
const deposited = parseFloat(depositedStr.replace(/,/g, ''))
const pctOfLimit = ((deposited / limit) * 100).toFixed(1)📝 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.
| const limit = tierLimits[limitStr] || 0 | |
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | |
| const pctOfLimit = ((deposited / limit) * 100).toFixed(1) | |
| const limit = tierLimits[limitStr] || 0 | |
| if (limit === 0) continue | |
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | |
| const pctOfLimit = ((deposited / limit) * 100).toFixed(1) |
🤖 Prompt for AI Agents
In `@scripts/simulation/analyzeUtilization.ts` around lines 38 - 40, The current
calculation for pctOfLimit divides deposited by limit and can produce
Infinity/NaN when tierLimits[limitStr] is undefined (limit === 0); update the
logic around the variables limit, limitStr, tierLimits, deposited and pctOfLimit
to guard against a zero/undefined limit: check if limit is a positive number
before doing (deposited / limit) and if not, set pctOfLimit to a safe default
string (e.g., "0.0" or "N/A") or handle it explicitly so no division occurs.
| const totalNotProfit = notProfitableMaxed + notProfitableNotMaxed | ||
| const pctNotMaxed = ((notProfitableNotMaxed / totalNotProfit) * 100).toFixed(1) | ||
| console.log(`\n📊 Of ${totalNotProfit} unprofitable users, ${notProfitableNotMaxed} (${pctNotMaxed}%) did NOT max their tier limit`) |
There was a problem hiding this comment.
Division by zero if no unprofitable users exist.
If totalNotProfit is 0, the percentage calculation on line 68 will produce NaN.
Proposed fix
const totalNotProfit = notProfitableMaxed + notProfitableNotMaxed
-const pctNotMaxed = ((notProfitableNotMaxed / totalNotProfit) * 100).toFixed(1)
-console.log(`\n📊 Of ${totalNotProfit} unprofitable users, ${notProfitableNotMaxed} (${pctNotMaxed}%) did NOT max their tier limit`)
+if (totalNotProfit > 0) {
+ const pctNotMaxed = ((notProfitableNotMaxed / totalNotProfit) * 100).toFixed(1)
+ console.log(`\n📊 Of ${totalNotProfit} unprofitable users, ${notProfitableNotMaxed} (${pctNotMaxed}%) did NOT max their tier limit`)
+} else {
+ console.log(`\n📊 No unprofitable users found.`)
+}📝 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.
| const totalNotProfit = notProfitableMaxed + notProfitableNotMaxed | |
| const pctNotMaxed = ((notProfitableNotMaxed / totalNotProfit) * 100).toFixed(1) | |
| console.log(`\n📊 Of ${totalNotProfit} unprofitable users, ${notProfitableNotMaxed} (${pctNotMaxed}%) did NOT max their tier limit`) | |
| const totalNotProfit = notProfitableMaxed + notProfitableNotMaxed | |
| if (totalNotProfit > 0) { | |
| const pctNotMaxed = ((notProfitableNotMaxed / totalNotProfit) * 100).toFixed(1) | |
| console.log(`\n📊 Of ${totalNotProfit} unprofitable users, ${notProfitableNotMaxed} (${pctNotMaxed}%) did NOT max their tier limit`) | |
| } else { | |
| console.log(`\n📊 No unprofitable users found.`) | |
| } |
🤖 Prompt for AI Agents
In `@scripts/simulation/analyzeUtilization.ts` around lines 67 - 69, The
pctNotMaxed calculation can produce NaN when totalNotProfit is 0; update the
code that computes pctNotMaxed (currently using totalNotProfit and
notProfitableNotMaxed) to guard against division by zero by computing
pctNotMaxed only if totalNotProfit > 0 (e.g., use a conditional or ternary) and
otherwise set a safe formatted value like "0.0" (so toFixed is not called on
NaN), then use that safe value in the console.log.
| rows.push(['Total Original Fees Collected', `$${totalOriginalFees.toFixed(2)}`]) | ||
| rows.push(['Total Dynamic Fees (Proposed)', `$${totalDynamicFees.toFixed(2)}`]) | ||
| rows.push(['Fee Reduction', `${((1 - totalDynamicFees / totalOriginalFees) * 100).toFixed(1)}%`]) | ||
| rows.push([]) | ||
| rows.push(['Total VAPE Value Distributed', `$${totalVAPEValue.toFixed(2)}`]) | ||
| rows.push([]) | ||
| rows.push(['PROFITABILITY ANALYSIS (Paid Pass Users Only)']) | ||
| rows.push(['Original Pricing - Profitable Users', `${originalProfitable} / ${usersWithPaidPass} (${(originalProfitable / usersWithPaidPass * 100).toFixed(1)}%)`]) | ||
| rows.push(['Dynamic Pricing - Profitable Users', `${dynamicProfitable} / ${usersWithPaidPass} (${(dynamicProfitable / usersWithPaidPass * 100).toFixed(1)}%)`]) | ||
| rows.push(['Improvement', `+${dynamicProfitable - originalProfitable} users (+${((dynamicProfitable - originalProfitable) / usersWithPaidPass * 100).toFixed(1)}%)`]) | ||
| rows.push([]) | ||
| rows.push(['Average Original ROI', `${(totalVAPEValue / totalOriginalFees).toFixed(2)}x`]) | ||
| rows.push(['Average Dynamic ROI', `${(totalVAPEValue / totalDynamicFees).toFixed(2)}x`]) |
There was a problem hiding this comment.
Multiple division-by-zero risks in summary statistics.
Lines 232, 237–239, and 241–242 all divide by totalOriginalFees, usersWithPaidPass, or totalDynamicFees. If no paid-pass users exist, all these values are 0, producing NaN/Infinity in the CSV output. Guard these divisions.
Example guard
- rows.push(['Fee Reduction', `${((1 - totalDynamicFees / totalOriginalFees) * 100).toFixed(1)}%`])
+ rows.push(['Fee Reduction', totalOriginalFees > 0 ? `${((1 - totalDynamicFees / totalOriginalFees) * 100).toFixed(1)}%` : 'N/A'])Apply similar guards for usersWithPaidPass divisions on lines 237–239, 241–242, and the console logs on 277–279.
🤖 Prompt for AI Agents
In `@scripts/simulation/exportSeasonAnalysis.ts` around lines 230 - 242, Guard all
divisions in the summary by checking denominators before computing ratios: when
building rows with totalOriginalFees, totalDynamicFees, usersWithPaidPass, and
when computing ROI/percentages for originalProfitable/dynamicProfitable use
conditional logic to avoid dividing by zero (e.g., if totalOriginalFees or
totalDynamicFees is 0, output "0.00x" or "N/A" for ROI; if usersWithPaidPass is
0, output "0 / 0 (0.0%)" or "N/A" for percentages and improvement), updating the
code that pushes rows (references: rows.push lines that use totalOriginalFees,
totalDynamicFees, totalVAPEValue, usersWithPaidPass, originalProfitable,
dynamicProfitable) and apply the same guards to the console.log statements
around the later summary output (console logs referenced at ~277–279) so no
division-by-zero produces NaN/Infinity.
| tier, | ||
| originalPriceUSDC: originalPrice, | ||
| dynamicPriceUSDC: Math.max(dynamicPriceUSDC, 0.01), | ||
| expectedVAPE: expectedVAPENumber, | ||
| expectedValueUSD, | ||
| estimatedROI: actualROI, | ||
| depositLimitVPND: tier === 10 ? 'Unlimited (using 50M for calc)' : formatUnits(depositLimit, 18), | ||
| } | ||
| } |
There was a problem hiding this comment.
Tier 0 (free) users incorrectly get a minimum fee of $0.01.
Math.max(dynamicPriceUSDC, 0.01) on line 190 forces a non-zero price even for tier 0, where originalPrice is 0 and users shouldn't be charged. The same issue exists in calculateSimpleDynamicPrice at line 209.
Proposed fix for both functions
// In calculateDynamicPrice (line 190):
- dynamicPriceUSDC: Math.max(dynamicPriceUSDC, 0.01),
+ dynamicPriceUSDC: originalPrice === 0 ? 0 : Math.max(dynamicPriceUSDC, 0.01),
// In calculateSimpleDynamicPrice (line 209):
- dynamicPrice: Math.max(dynamicPrice, 0.01),
+ dynamicPrice: originalPrice === 0 ? 0 : Math.max(dynamicPrice, 0.01),📝 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.
| tier, | |
| originalPriceUSDC: originalPrice, | |
| dynamicPriceUSDC: Math.max(dynamicPriceUSDC, 0.01), | |
| expectedVAPE: expectedVAPENumber, | |
| expectedValueUSD, | |
| estimatedROI: actualROI, | |
| depositLimitVPND: tier === 10 ? 'Unlimited (using 50M for calc)' : formatUnits(depositLimit, 18), | |
| } | |
| } | |
| tier, | |
| originalPriceUSDC: originalPrice, | |
| dynamicPriceUSDC: originalPrice === 0 ? 0 : Math.max(dynamicPriceUSDC, 0.01), | |
| expectedVAPE: expectedVAPENumber, | |
| expectedValueUSD, | |
| estimatedROI: actualROI, | |
| depositLimitVPND: tier === 10 ? 'Unlimited (using 50M for calc)' : formatUnits(depositLimit, 18), | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@scripts/simulation/miningPassPricing.ts` around lines 188 - 196, The
minimum-price clamp is forcing tier 0 (free) passes to cost $0.01; update the
logic in miningPassPricing.ts so the Math.max(..., 0.01) floor is only applied
for paid tiers. Specifically, where dynamicPriceUSDC is set (and in
calculateSimpleDynamicPrice), change the assignment so that if tier === 0 you
keep dynamicPriceUSDC (or originalPrice) as 0, otherwise apply the Math.max(...,
0.01) clamp; adjust any related fields (e.g., dynamicPriceUSDC,
originalPriceUSDC) so tier 0 remains zero-priced and paid tiers retain the $0.01
minimum.
| const limit = tierLimits[limitStr] || 0 | ||
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | ||
| const utilization = (deposited / limit) * 100 |
There was a problem hiding this comment.
Division by zero when limit is 0.
Same pattern as in analyzeUtilization.ts: if tierLimits[limitStr] doesn't match, limit is 0 and utilization becomes Infinity.
Proposed fix
const limit = tierLimits[limitStr] || 0
+ if (limit === 0) continue
const deposited = parseFloat(depositedStr.replace(/,/g, ''))📝 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.
| const limit = tierLimits[limitStr] || 0 | |
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | |
| const utilization = (deposited / limit) * 100 | |
| const limit = tierLimits[limitStr] || 0 | |
| if (limit === 0) continue | |
| const deposited = parseFloat(depositedStr.replace(/,/g, '')) | |
| const utilization = (deposited / limit) * 100 |
🤖 Prompt for AI Agents
In `@scripts/simulation/unprofitableAnalysis.ts` around lines 45 - 47, The
calculation of utilization can divide by zero when tierLimits[limitStr] is
missing (limit becomes 0), causing Infinity; update the logic around const limit
= tierLimits[limitStr] || 0 and the utilization calculation to guard against
zero/NaN: compute deposited from depositedStr as before, check if limit is a
positive number (e.g., > 0) before doing (deposited / limit) * 100 and otherwise
set utilization to 0 (or null/NaN per project convention), mirroring the safe
handling used in analyzeUtilization.ts and ensure any downstream code can handle
the fallback value.
| let grandTotalLoss = 0 | ||
| for (const tier of Object.keys(tierStats).map(Number).sort((a, b) => a - b)) { | ||
| const stats = tierStats[tier] | ||
| const avgROI = (stats.rois.reduce((a, b) => a + b, 0) / stats.count).toFixed(2) | ||
| const avgUtil = (stats.utils.reduce((a, b) => a + b, 0) / stats.count).toFixed(1) | ||
| const avgLoss = (stats.totalLoss / stats.count).toFixed(2) | ||
| grandTotalLoss += stats.totalLoss | ||
| console.log(` ${tier} | ${String(stats.count).padStart(2)} | $${stats.totalLoss.toFixed(2).padStart(7)} | $${avgLoss.padStart(5)} | ${avgROI}x | ${avgUtil}%`) | ||
| } | ||
|
|
||
| console.log('─'.repeat(70)) | ||
| console.log(`TOTAL | ${unprofitableUsers.length} | $${grandTotalLoss.toFixed(2).padStart(7)} | $${(grandTotalLoss / unprofitableUsers.length).toFixed(2).padStart(5)} |`) |
There was a problem hiding this comment.
Division by zero if unprofitableUsers is empty.
Line 104 divides grandTotalLoss by unprofitableUsers.length. If the array is empty (e.g., all users are profitable), this produces NaN.
Proposed fix
-console.log(`TOTAL | ${unprofitableUsers.length} | $${grandTotalLoss.toFixed(2).padStart(7)} | $${(grandTotalLoss / unprofitableUsers.length).toFixed(2).padStart(5)} |`)
+const avgLossTotal = unprofitableUsers.length > 0 ? (grandTotalLoss / unprofitableUsers.length).toFixed(2) : '0.00'
+console.log(`TOTAL | ${unprofitableUsers.length} | $${grandTotalLoss.toFixed(2).padStart(7)} | $${avgLossTotal.padStart(5)} |`)📝 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.
| let grandTotalLoss = 0 | |
| for (const tier of Object.keys(tierStats).map(Number).sort((a, b) => a - b)) { | |
| const stats = tierStats[tier] | |
| const avgROI = (stats.rois.reduce((a, b) => a + b, 0) / stats.count).toFixed(2) | |
| const avgUtil = (stats.utils.reduce((a, b) => a + b, 0) / stats.count).toFixed(1) | |
| const avgLoss = (stats.totalLoss / stats.count).toFixed(2) | |
| grandTotalLoss += stats.totalLoss | |
| console.log(` ${tier} | ${String(stats.count).padStart(2)} | $${stats.totalLoss.toFixed(2).padStart(7)} | $${avgLoss.padStart(5)} | ${avgROI}x | ${avgUtil}%`) | |
| } | |
| console.log('─'.repeat(70)) | |
| console.log(`TOTAL | ${unprofitableUsers.length} | $${grandTotalLoss.toFixed(2).padStart(7)} | $${(grandTotalLoss / unprofitableUsers.length).toFixed(2).padStart(5)} |`) | |
| let grandTotalLoss = 0 | |
| for (const tier of Object.keys(tierStats).map(Number).sort((a, b) => a - b)) { | |
| const stats = tierStats[tier] | |
| const avgROI = (stats.rois.reduce((a, b) => a + b, 0) / stats.count).toFixed(2) | |
| const avgUtil = (stats.utils.reduce((a, b) => a + b, 0) / stats.count).toFixed(1) | |
| const avgLoss = (stats.totalLoss / stats.count).toFixed(2) | |
| grandTotalLoss += stats.totalLoss | |
| console.log(` ${tier} | ${String(stats.count).padStart(2)} | $${stats.totalLoss.toFixed(2).padStart(7)} | $${avgLoss.padStart(5)} | ${avgROI}x | ${avgUtil}%`) | |
| } | |
| console.log('─'.repeat(70)) | |
| const avgLossTotal = unprofitableUsers.length > 0 ? (grandTotalLoss / unprofitableUsers.length).toFixed(2) : '0.00' | |
| console.log(`TOTAL | ${unprofitableUsers.length} | $${grandTotalLoss.toFixed(2).padStart(7)} | $${avgLossTotal.padStart(5)} |`) |
🤖 Prompt for AI Agents
In `@scripts/simulation/unprofitableAnalysis.ts` around lines 93 - 104, The final
console.log divides grandTotalLoss by unprofitableUsers.length which can be
zero; guard that division by computing an averageLoss variable like avgPerUser =
unprofitableUsers.length ? grandTotalLoss / unprofitableUsers.length : 0 and use
that in the TOTAL line formatting so you never produce NaN; update the TOTAL
console.log to use avgPerUser (formatted toFixed(2) and padStart as before) and
reference grandTotalLoss and unprofitableUsers in the same scope.
| /// @dev Base fees (original fees set at deployment, used for floor price calculation) | ||
| mapping(uint256 => uint256) baseMiningPassTierToFee; | ||
| /// @dev Floor price percentage in basis points (e.g., 2500 = 25%) | ||
| uint256 miningPassFeeFloorBps; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the AppStorage.sol file to verify the struct layout
fd AppStorage.sol --type fRepository: VaporFi/liquid-mining
Length of output: 93
🏁 Script executed:
# Once we have the file, let's check its full structure around the storage fields
git ls-files | grep -i appstorageRepository: VaporFi/liquid-mining
Length of output: 264
🏁 Script executed:
# Search for deployment references and Season 30 mentions
rg "Season 30|0x[A-Fa-f0-9]{40}" --type-add 'sol:*.sol' --type-add 'ts:*.ts' --type-add 'json:*.json' -g '!node_modules' | head -50Repository: VaporFi/liquid-mining
Length of output: 50378
🏁 Script executed:
# Read AppStorage.sol to see the struct definition and field positions
wc -l src/libraries/AppStorage.sol && head -120 src/libraries/AppStorage.sol | tail -50Repository: VaporFi/liquid-mining
Length of output: 1950
🏁 Script executed:
# Get the full struct around the problematic lines
sed -n '50,110p' src/libraries/AppStorage.solRepository: VaporFi/liquid-mining
Length of output: 1899
Critical: Storage layout collision — new fields must be appended at the end of the struct, not inserted in the middle.
These two new fields (baseMiningPassTierToFee, miningPassFeeFloorBps) are inserted between the MINING PASS and GENERAL sections. In the Diamond proxy pattern, all facets share a single storage layout via AppStorage. Since this diamond is already deployed (confirmed by active Season 30 data and references in multiple script files), inserting fields mid-struct shifts the storage slots of every subsequent field (depositToken, rewardToken, feeToken, stratosphereAddress, reentrancyGuardStatus, emissionsManager, gelatoExecutor, isSeasonClaimed), causing silent data corruption on upgrade.
New storage fields must be appended after the last field in the struct (after isSeasonClaimed).
Proposed fix
address[] miningPassFeeReceivers;
uint256[] miningPassFeeReceiversShares;
- /// `@dev` Base fees (original fees set at deployment, used for floor price calculation)
- mapping(uint256 => uint256) baseMiningPassTierToFee;
- /// `@dev` Floor price percentage in basis points (e.g., 2500 = 25%)
- uint256 miningPassFeeFloorBps;
///////////////
/// GENERAL ///
///////////////
address depositToken;
...
mapping(uint256 => bool) isSeasonClaimed;
+ ///////////////////////////
+ /// MINING PASS PRICING ///
+ ///////////////////////////
+ /// `@dev` Base fees (original fees set at deployment, used for floor price calculation)
+ mapping(uint256 => uint256) baseMiningPassTierToFee;
+ /// `@dev` Floor price percentage in basis points (e.g., 2500 = 25%)
+ uint256 miningPassFeeFloorBps;
}🤖 Prompt for AI Agents
In `@src/libraries/AppStorage.sol` around lines 80 - 83, The two new storage
fields baseMiningPassTierToFee and miningPassFeeFloorBps were inserted
mid-struct in AppStorage causing a storage layout collision; move these
declarations out of the MINING PASS/GENERAL region and append them after the
last existing storage variable isSeasonClaimed in the AppStorage struct so they
occupy new slots, update any nearby comments to reflect their new position, and
ensure no other fields are reordered or removed.
| mapping(uint256 => uint256) __deprecated_unlockFeeDiscount; // slot 8 | ||
| mapping(uint256 => uint256) __deprecated_depositDiscount; // slot 9 | ||
| mapping(uint256 => uint256) __deprecated_restakeDiscount; // slot 10 |
There was a problem hiding this comment.
comment for self: try to remove it while fixing collision on fuji
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deployments/DepositFacet.json (1)
84-159:⚠️ Potential issue | 🟡 MinorHardhat ABI is missing the
DepositFacet__Less_Than_One_Dayerror defined in the source code.The source code defines this error at line 17 and uses it at line 77, but the hardhat artifact ABI is missing it. This inconsistency will cause decoding failures when this error is reverted in local tests. Update the hardhat artifact to match the current source code.
deployments/MiningPassFacet.json (1)
133-234:⚠️ Potential issue | 🟠 MajorHardhat ABI is missing the new
getMiningPassFeefunction andMiningPassFacet__InvalidFeeReceiverserror.The
fujiandavalanchesections both include the newMiningPassFacet__InvalidFeeReceiverserror andgetMiningPassFee(address,uint256)function, but thehardhatsection still has the old ABI without these entries. This means local development and Hardhat-based tests consuming this artifact won't have access to the new dynamic pricing interface.Please regenerate or update the hardhat deployment artifact to match the fuji/avalanche ABI.
🤖 Fix all issues with AI agents
In `@deployments/MiningPassFacet.json`:
- Around line 121-122: Fuji's deployed bytecode references storage slot 0x19
(depositToken) instead of 0x17 for s.miningPassTierToFee[_tier], indicating
compilation against an out-of-date AppStorage layout; recompile the
MiningPassFacet (symbol: s.miningPassTierToFee and the contract/source
MiningPassFacet) against the current AppStorage.sol, regenerate the Fuji
artifact (replace the bytecode in MiningPassFacet.json), ensure identical
compiler version, settings and ABI across networks, compare the resulting
storage layout mapping (slots for miningPassTierToFee and depositToken) with
Avalanche's artifact, and redeploy Fuji only after confirming on-chain storage
expectations match the new bytecode to avoid storage collisions.
In `@src/facets/DiamondManagerFacet.sol`:
- Around line 188-205: The current setBaseMiningPassFees function updates
s.baseMiningPassTierToFee without ensuring existing s.miningPassTierToFee
entries still meet the new floor (computed as baseFee * floorBps / 10000),
leading to inconsistent state; update setBaseMiningPassFees to, for each tier i,
compute uint256 floor = fees[i] * s.floorBps / 10000 and either revert if
s.miningPassTierToFee[i] < floor (preferred) or set s.miningPassTierToFee[i] =
floor to enforce it immediately; reference the functions/vars
setBaseMiningPassFees, s.baseMiningPassTierToFee, s.miningPassTierToFee,
floorBps and keep behavior consistent with
setMiningPassTierFee/setMiningPassFees expectations.
In `@tasks/diamondInfo.ts`:
- Around line 114-117: The destructured variable name depositTimestamp is
misleading because dm.getUserDepositAmount(user, seasonId) returns
(depositAmount, depositPoints); update the destructuring in tasks/diamondInfo.ts
to use depositPoints (or drop the second element with an underscore if unused)
so it matches the returned semantics from getUserDepositAmount and avoid
confusion for future maintainers; ensure you reference the function name
getUserDepositAmount and the variables depositAmount/depositPoints when making
the change.
In `@tasks/startSeason.ts`:
- Around line 5-7: The local constant FLOOR_BPS (2500) in tasks/startSeason.ts
can drift from the on-chain miningPassFeeFloorBps; update the flow to fetch the
canonical value from the contract via getMiningPassFeeFloorBps() and use that
returned value in calculateDynamicFees instead of the hardcoded FLOOR_BPS; make
sure to call the contract getter before running startSeason logic (or pass the
fetched floor into calculateDynamicFees), handle the async call and any possible
failure, and remove or deprecate the hardcoded FLOOR_BPS constant so all fee
floor checks use the on-chain value.
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 126-151: _issue_: _upgradeFacets currently only uses
IDiamondCut.FacetCutAction.Replace for newDMF/newMPF so any brand-new selectors
(the 10 dynamic-pricing functions like setMiningPassFees, setBaseMiningPassFees,
setMiningPassFeeFloor, getMiningPassTierFee, getBaseMiningPassTierFee,
getMiningPassTierFloorFee, getMiningPassFeeFloorBps, getAllMiningPassFees,
getMiningPassTierDepositLimit, setMiningPassTierFee) are never registered and
later calls in _reinitializeMiningPassData and tests will revert. Fix: update
_upgradeFacets to include an additional IDiamondCut.FacetCut with action
IDiamondCut.FacetCutAction.Add that registers the new function selectors from
the new facet instances (newDMF/newMPF); adjust the cuts array size (e.g., from
2 to 3) and populate the Add cut with the bytes4[] of the new selectors for
whichever facet defines those functions so the diamond actually adds those
functions instead of only replacing existing selectors.
🧹 Nitpick comments (5)
scripts/initDynamicPricing.ts (1)
19-47: Consider adding an idempotency check before overwriting on-chain state.Re-running this script will silently overwrite existing pricing configuration. For a production initialization script, consider checking current values first (e.g., reading
getBaseMiningPassTierFee(1)) and prompting or aborting if already set.src/facets/DiamondManagerFacet.sol (1)
472-519: Getter functions don't validate tier bounds.
getMiningPassTierFee,getBaseMiningPassTierFee,getMiningPassTierFloorFee, andgetMiningPassTierDepositLimitwill silently return 0 (or a computed 0) for anytier > 10. This is harmless for view functions but could mislead off-chain consumers into thinking tier 11+ exists with zero fees.Consider adding the same
if (tier > 10) revert DiamondManagerFacet__InvalidTier();guard used insetMiningPassTierFee.tasks/startSeason.ts (2)
9-17:fetchVAPEPricehas no timeout or retry — can hang indefinitely.If CoinGecko is slow or unreachable, the task will block forever. Consider adding an
AbortControllertimeout:Proposed fix
async function fetchVAPEPrice(): Promise<number> { + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 10_000) const response = await fetch( - 'https://api.coingecko.com/api/v3/simple/price?ids=vaporfi&vs_currencies=usd' + 'https://api.coingecko.com/api/v3/simple/price?ids=vaporfi&vs_currencies=usd', + { signal: controller.signal } ) + clearTimeout(timeout)
108-131: No error handling around on-chain transactions.If
setMiningPassFeessucceeds butstartNewSeasonWithEndTimestampreverts, the fees are updated but no season is started — leaving the system in an inconsistent state. Consider wrapping the transaction sequence in a try/catch with clear messaging about partial completion, or at minimum log which step failed.tasks/diamondInfo.ts (1)
72-86: N+1 query pattern — 11 iterations each making 4 RPC calls.Each tier iteration makes 4 separate contract calls. For a CLI info tool this is acceptable, but
getAllMiningPassFees()is already available and could reduce calls. Consider batching where possible for better UX on slow RPC endpoints.
| const [depositAmount, depositTimestamp] = await dm.getUserDepositAmount( | ||
| user, | ||
| seasonId | ||
| ) |
There was a problem hiding this comment.
Misleading destructured variable name: depositTimestamp is actually depositPoints.
getUserDepositAmount returns (depositAmount, depositPoints) (see DiamondManagerFacet.sol Line 349-351), not a timestamp. The variable depositTimestamp is unused so there's no functional bug, but it could confuse future maintainers.
Proposed fix
- const [depositAmount, depositTimestamp] = await dm.getUserDepositAmount(
+ const [depositAmount, _depositPoints] = await dm.getUserDepositAmount(
user,
seasonId
)🤖 Prompt for AI Agents
In `@tasks/diamondInfo.ts` around lines 114 - 117, The destructured variable name
depositTimestamp is misleading because dm.getUserDepositAmount(user, seasonId)
returns (depositAmount, depositPoints); update the destructuring in
tasks/diamondInfo.ts to use depositPoints (or drop the second element with an
underscore if unused) so it matches the returned semantics from
getUserDepositAmount and avoid confusion for future maintainers; ensure you
reference the function name getUserDepositAmount and the variables
depositAmount/depositPoints when making the change.
| const REFERENCE_VAPE_PRICE = 0.606 | ||
| const FLOOR_BPS = 2500 // 25% | ||
| const NUM_TIERS = 11 |
There was a problem hiding this comment.
FLOOR_BPS is hardcoded and may diverge from on-chain value.
The task uses FLOOR_BPS = 2500 locally in calculateDynamicFees, but the on-chain contract has its own miningPassFeeFloorBps. If the on-chain floor is changed independently, the locally computed fees might pass the local floor check but fail the on-chain check (or vice versa — pass on-chain but not represent the intended floor locally).
Consider reading getMiningPassFeeFloorBps() from the contract instead:
Proposed fix
-const FLOOR_BPS = 2500 // 25%
...
+ // ── 2. Read base fees and floor from contract ────────────────
+ const floorBps = Number(await DiamondManagerFacet.getMiningPassFeeFloorBps())
const baseFees: bigint[] = []
...
- const dynamicFees = calculateDynamicFees(baseFees, currentVAPEPrice, FLOOR_BPS)
+ const dynamicFees = calculateDynamicFees(baseFees, currentVAPEPrice, floorBps)🤖 Prompt for AI Agents
In `@tasks/startSeason.ts` around lines 5 - 7, The local constant FLOOR_BPS (2500)
in tasks/startSeason.ts can drift from the on-chain miningPassFeeFloorBps;
update the flow to fetch the canonical value from the contract via
getMiningPassFeeFloorBps() and use that returned value in calculateDynamicFees
instead of the hardcoded FLOOR_BPS; make sure to call the contract getter before
running startSeason logic (or pass the fetched floor into calculateDynamicFees),
handle the async call and any possible failure, and remove or deprecate the
hardcoded FLOOR_BPS constant so all fee floor checks use the on-chain value.
| function _upgradeFacets() internal { | ||
| DiamondManagerFacet newDMF = new DiamondManagerFacet(); | ||
| MiningPassFacet newMPF = new MiningPassFacet(); | ||
|
|
||
| // Use the loupe to find existing facet addresses via known selectors | ||
| address oldDmfAddr = loupe.facetAddress(DiamondManagerFacet.setDepositToken.selector); | ||
| address oldMpfAddr = loupe.facetAddress(MiningPassFacet.purchase.selector); | ||
|
|
||
| // Get ALL registered selectors for those facets | ||
| bytes4[] memory dmfSelectors = loupe.facetFunctionSelectors(oldDmfAddr); | ||
| bytes4[] memory mpfSelectors = loupe.facetFunctionSelectors(oldMpfAddr); | ||
|
|
||
| console.log("DMF selectors to replace:", dmfSelectors.length); | ||
| console.log("MPF selectors to replace:", mpfSelectors.length); | ||
|
|
||
| IDiamondCut.FacetCut[] memory cuts = new IDiamondCut.FacetCut[](2); | ||
| cuts[0] = IDiamondCut.FacetCut({ | ||
| facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: dmfSelectors | ||
| }); | ||
| cuts[1] = IDiamondCut.FacetCut({ | ||
| facetAddress: address(newMPF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: mpfSelectors | ||
| }); | ||
|
|
||
| vm.prank(owner); | ||
| IDiamondCut(DIAMOND).diamondCut(cuts, address(0), ""); | ||
| } |
There was a problem hiding this comment.
_upgradeFacets only replaces existing selectors — new dynamic pricing selectors are never added.
FacetCutAction.Replace only updates the facet address for selectors already registered in the diamond. The 10 new functions (setMiningPassFees, setBaseMiningPassFees, setMiningPassFeeFloor, getMiningPassTierFee, getBaseMiningPassTierFee, getMiningPassTierFloorFee, getMiningPassFeeFloorBps, getAllMiningPassFees, getMiningPassTierDepositLimit, setMiningPassTierFee) don't exist on the deployed Fuji diamond, so they won't be registered. Calls to these functions in _reinitializeMiningPassData (line 168-170) and assertions (line 77, 81) will revert with "Diamond: Function does not exist".
You need an additional FacetCutAction.Add cut for the new selectors:
Proposed fix
- IDiamondCut.FacetCut[] memory cuts = new IDiamondCut.FacetCut[](2);
+ // New selectors that don't exist on the current diamond
+ bytes4[] memory newSelectors = new bytes4[](10);
+ newSelectors[0] = DiamondManagerFacet.setMiningPassFees.selector;
+ newSelectors[1] = DiamondManagerFacet.setMiningPassTierFee.selector;
+ newSelectors[2] = DiamondManagerFacet.setMiningPassFeeFloor.selector;
+ newSelectors[3] = DiamondManagerFacet.getMiningPassTierFee.selector;
+ newSelectors[4] = DiamondManagerFacet.getBaseMiningPassTierFee.selector;
+ newSelectors[5] = DiamondManagerFacet.getMiningPassTierFloorFee.selector;
+ newSelectors[6] = DiamondManagerFacet.getMiningPassFeeFloorBps.selector;
+ newSelectors[7] = DiamondManagerFacet.getAllMiningPassFees.selector;
+ newSelectors[8] = DiamondManagerFacet.getMiningPassTierDepositLimit.selector;
+ newSelectors[9] = DiamondManagerFacet.setBaseMiningPassFees.selector;
+
+ IDiamondCut.FacetCut[] memory cuts = new IDiamondCut.FacetCut[](3);
cuts[0] = IDiamondCut.FacetCut({
facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: dmfSelectors
});
cuts[1] = IDiamondCut.FacetCut({
facetAddress: address(newMPF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: mpfSelectors
});
+ cuts[2] = IDiamondCut.FacetCut({
+ facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Add, functionSelectors: newSelectors
+ });📝 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.
| function _upgradeFacets() internal { | |
| DiamondManagerFacet newDMF = new DiamondManagerFacet(); | |
| MiningPassFacet newMPF = new MiningPassFacet(); | |
| // Use the loupe to find existing facet addresses via known selectors | |
| address oldDmfAddr = loupe.facetAddress(DiamondManagerFacet.setDepositToken.selector); | |
| address oldMpfAddr = loupe.facetAddress(MiningPassFacet.purchase.selector); | |
| // Get ALL registered selectors for those facets | |
| bytes4[] memory dmfSelectors = loupe.facetFunctionSelectors(oldDmfAddr); | |
| bytes4[] memory mpfSelectors = loupe.facetFunctionSelectors(oldMpfAddr); | |
| console.log("DMF selectors to replace:", dmfSelectors.length); | |
| console.log("MPF selectors to replace:", mpfSelectors.length); | |
| IDiamondCut.FacetCut[] memory cuts = new IDiamondCut.FacetCut[](2); | |
| cuts[0] = IDiamondCut.FacetCut({ | |
| facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: dmfSelectors | |
| }); | |
| cuts[1] = IDiamondCut.FacetCut({ | |
| facetAddress: address(newMPF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: mpfSelectors | |
| }); | |
| vm.prank(owner); | |
| IDiamondCut(DIAMOND).diamondCut(cuts, address(0), ""); | |
| } | |
| function _upgradeFacets() internal { | |
| DiamondManagerFacet newDMF = new DiamondManagerFacet(); | |
| MiningPassFacet newMPF = new MiningPassFacet(); | |
| // Use the loupe to find existing facet addresses via known selectors | |
| address oldDmfAddr = loupe.facetAddress(DiamondManagerFacet.setDepositToken.selector); | |
| address oldMpfAddr = loupe.facetAddress(MiningPassFacet.purchase.selector); | |
| // Get ALL registered selectors for those facets | |
| bytes4[] memory dmfSelectors = loupe.facetFunctionSelectors(oldDmfAddr); | |
| bytes4[] memory mpfSelectors = loupe.facetFunctionSelectors(oldMpfAddr); | |
| console.log("DMF selectors to replace:", dmfSelectors.length); | |
| console.log("MPF selectors to replace:", mpfSelectors.length); | |
| // New selectors that don't exist on the current diamond | |
| bytes4[] memory newSelectors = new bytes4[](10); | |
| newSelectors[0] = DiamondManagerFacet.setMiningPassFees.selector; | |
| newSelectors[1] = DiamondManagerFacet.setMiningPassTierFee.selector; | |
| newSelectors[2] = DiamondManagerFacet.setMiningPassFeeFloor.selector; | |
| newSelectors[3] = DiamondManagerFacet.getMiningPassTierFee.selector; | |
| newSelectors[4] = DiamondManagerFacet.getBaseMiningPassTierFee.selector; | |
| newSelectors[5] = DiamondManagerFacet.getMiningPassTierFloorFee.selector; | |
| newSelectors[6] = DiamondManagerFacet.getMiningPassFeeFloorBps.selector; | |
| newSelectors[7] = DiamondManagerFacet.getAllMiningPassFees.selector; | |
| newSelectors[8] = DiamondManagerFacet.getMiningPassTierDepositLimit.selector; | |
| newSelectors[9] = DiamondManagerFacet.setBaseMiningPassFees.selector; | |
| IDiamondCut.FacetCut[] memory cuts = new IDiamondCut.FacetCut[](3); | |
| cuts[0] = IDiamondCut.FacetCut({ | |
| facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: dmfSelectors | |
| }); | |
| cuts[1] = IDiamondCut.FacetCut({ | |
| facetAddress: address(newMPF), action: IDiamondCut.FacetCutAction.Replace, functionSelectors: mpfSelectors | |
| }); | |
| cuts[2] = IDiamondCut.FacetCut({ | |
| facetAddress: address(newDMF), action: IDiamondCut.FacetCutAction.Add, functionSelectors: newSelectors | |
| }); | |
| vm.prank(owner); | |
| IDiamondCut(DIAMOND).diamondCut(cuts, address(0), ""); | |
| } |
🤖 Prompt for AI Agents
In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 126 - 151, _issue_:
_upgradeFacets currently only uses IDiamondCut.FacetCutAction.Replace for
newDMF/newMPF so any brand-new selectors (the 10 dynamic-pricing functions like
setMiningPassFees, setBaseMiningPassFees, setMiningPassFeeFloor,
getMiningPassTierFee, getBaseMiningPassTierFee, getMiningPassTierFloorFee,
getMiningPassFeeFloorBps, getAllMiningPassFees, getMiningPassTierDepositLimit,
setMiningPassTierFee) are never registered and later calls in
_reinitializeMiningPassData and tests will revert. Fix: update _upgradeFacets to
include an additional IDiamondCut.FacetCut with action
IDiamondCut.FacetCutAction.Add that registers the new function selectors from
the new facet instances (newDMF/newMPF); adjust the cuts array size (e.g., from
2 to 3) and populate the Add cut with the bytes4[] of the new selectors for
whichever facet defines those functions so the diamond actually adds those
functions instead of only replacing existing selectors.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/upgradeInitializers/FujiMigrationInit.sol (3)
63-93: Slot 28 is not zeroed in the assembly cleanup block.Slots 27, 29, and 30 are explicitly cleared, but slot 28 is skipped with a comment that
miningPassFeeFloorBps"will be set below" (line 164). While the transaction is atomic so the stalestratosphereAddressvalue will be overwritten, for consistency and defensive coding it's cleaner to also zero slot 28 alongside its neighbors—especially since this is a one-shot migration and the cost is a singlesstore.Proposed fix
// Clear old slots that are no longer used as scalars // Slot 27 is now baseMiningPassTierToFee (mapping base = 0) sstore(27, 0) // Slot 28 is now miningPassFeeFloorBps (will be set below) + sstore(28, 0) // Slot 29 is now gelatoExecutor (address, leave 0 for now) sstore(29, 0) // Slot 30 is now isSeasonClaimed (mapping base = 0) sstore(30, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/upgradeInitializers/FujiMigrationInit.sol` around lines 63 - 93, The assembly cleanup block in FujiMigrationInit.sol currently clears storage slots 27, 29, and 30 but skips slot 28 (commenting that miningPassFeeFloorBps will be set later), leaving stratosphereAddress stale until overwritten; add an sstore to explicitly zero slot 28 in the same assembly cleanup sequence so slot 28 is cleared defensively along with 27/29/30 before subsequent writes to miningPassFeeFloorBps or stratosphereAddress, referencing the existing assembly block that performs sstore for slots 27, 29, and 30 and the storage variables stratosphereAddress and miningPassFeeFloorBps.
39-41: No re-execution guard for this one-time migration.The comment states this must be called "exactly once," but there is no on-chain mechanism (e.g., a boolean flag checked and set at the top of
init) to enforce that. While the diamond owner controls whendiamondCutis called, an accidental re-execution would silently overwrite storage—particularly risky for the scalar slot relocations, which would read already-migrated (or zeroed) values and propagate them.For a testnet migration this is low-risk, but consider adding a simple guard.
Example guard
function init(Args memory _args) external { + // Prevent accidental re-execution + require(s.miningPassFeeFloorBps == 0, "FujiMigration: already executed"); + // ─── Step 1: Migrate GENERAL scalars ...This works because
miningPassFeeFloorBpsis a new field that starts at 0 and gets set to 2500 at the end of init. Any re-invocation would see 2500 and revert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/upgradeInitializers/FujiMigrationInit.sol` around lines 39 - 41, The init function in FujiMigrationInit.sol is missing a one-time execution guard, so re-running the migration can silently overwrite storage; add a guard at the top of init (e.g., a boolean _fujiMigrationInitialized or check the new scalar miningPassFeeFloorBps) that reverts if the migration already ran, and set that flag (or set miningPassFeeFloorBps to its post-migration value like 2500) at the end of init; reference the init function and the miningPassFeeFloorBps storage slot (or a new private bool) to implement the check-and-set pattern to prevent re-execution.
124-148: Fee tier values are duplicated betweenminingPassTierToFeeandbaseMiningPassTierToFee.Lines 125-135 and 138-148 are identical value assignments to two different mappings. This is intentional (base fees serve as the reference for dynamic pricing), but consider extracting a helper or loop to avoid the copy-paste and ensure they stay in sync if values change.
Example helper approach
+ // Helper: set fee for both current and base mappings + uint256[11] memory fees = [ + uint256(0), 0.5 * 1e6, 1 * 1e6, 2 * 1e6, 4 * 1e6, + 8 * 1e6, 15 * 1e6, 30 * 1e6, 50 * 1e6, 75 * 1e6, 100 * 1e6 + ]; + for (uint256 i = 0; i <= 10; i++) { + s.miningPassTierToFee[i] = fees[i]; + s.baseMiningPassTierToFee[i] = fees[i]; + } - // Re-initialize mining pass fees (prod slot 17, old Fuji slot 23) - s.miningPassTierToFee[0] = 0; - ... (all 22 lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/upgradeInitializers/FujiMigrationInit.sol` around lines 124 - 148, The assignments to miningPassTierToFee and baseMiningPassTierToFee are duplicated; extract a single helper (e.g., internal function initializeTierFees or _setTierFees) that accepts the tier keys and fee values and sets both mappings in one loop, then call that helper from the initializer instead of repeating the same value assignments so both mappings remain in sync.test/foundry/fork/FujiForkMiningPass.t.sol (1)
220-237: Overly broadcatchin_ensureActiveSeasoncan silently swallow post-migration errors.Any revert from
getSeasonEndTimestamp— including ABI mismatches, storage layout corruption, or out-of-gas — is silently caught and the helper just starts a new season. A more targeted check (e.g. catch the known "no season" revert string) would avoid masking real post-upgrade breakage.♻️ Suggested narrowing
try diamond.getSeasonEndTimestamp(seasonId) returns (uint256 endTs) { if (endTs <= block.timestamp) { vm.prank(owner); diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days); } - } catch { + } catch Error(string memory reason) { + // Only proceed if the season simply doesn't exist yet + bytes32 h = keccak256(bytes(reason)); + require( + h == keccak256("Season does not exist") || h == keccak256("no active season"), + string.concat("_ensureActiveSeason: unexpected revert: ", reason) + ); vm.prank(owner); diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 220 - 237, The catch in _ensureActiveSeason is too broad and may swallow real post-migration failures from getSeasonEndTimestamp; change the handler to only absorb the expected "no season" (or the exact revert string/selector your contract emits) and rethrow any other errors—locate _ensureActiveSeason and the call to getSeasonEndTimestamp and update the catch to inspect the error (e.g., check error.message or error.selector for the known "no season" marker) returning/starting a new season only when that specific condition matches, otherwise propagate the error so unexpected ABI/storage/OOG issues surface.
🤖 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/upgradeInitializers/FujiMigrationInit.sol`:
- Around line 99-122: The three loops that iterate over _args.*FeeReceivers and
index into the matching *FeeReceiversShares arrays must validate that the
receiver and share arrays have identical lengths before looping; add explicit
checks (e.g., require(_args.partnerFeeReceivers.length ==
_args.partnerFeeReceiversShares.length, "partner fee receivers/shares length
mismatch")) for each pair (partnerFeeReceivers vs partnerFeeReceiversShares,
platformFeeReceivers vs platformFeeReceiversShares, referralFeeReceivers vs
referralFeeReceiversShares) in the FujiMigrationInit migration function so you
revert with a clear error when lengths differ rather than allowing out-of-bounds
panics or silently dropping extra shares.
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 64-87: The test test_Fork_PreMigration_OldSlotLayout currently
uses vm.createSelectFork("fuji") which forks the current head and will break
once the migration runs; update the test to pin the fork to a specific block
number captured before the migration by calling vm.createSelectFork("fuji",
PRE_MIGRATION_BLOCK) (replace PRE_MIGRATION_BLOCK with the explicit block number
taken from the chain before the migration) so the assertions about slots 25–28
remain stable across CI runs.
- Around line 191-201: The test currently calls
loupe.facetFunctionSelectors(loupe.facetAddress(knownSelector)) without checking
for address(0), which causes diamondCut to fail cryptically if knownSelector
isn't registered; modify the test in FujiForkMiningPass.t.sol to first call
address facetAddr = loupe.facetAddress(knownSelector) and check facetAddr !=
address(0) before calling loupe.facetFunctionSelectors, and if it's address(0)
either skip/assert-fail with a clear message identifying knownSelector or throw
a descriptive revert/log so you never pass an empty selectors array into
diamondCut.
---
Duplicate comments:
In `@src/libraries/AppStorage.sol`:
- Around line 90-101: The new fields baseMiningPassTierToFee and
miningPassFeeFloorBps were inserted before existing storage entries causing
gelatoExecutor and isSeasonClaimed to shift and collide with deployed storage;
move the two new variables to be declared after isSeasonClaimed in
AppStorage.sol so the original ordering (gelatoExecutor then isSeasonClaimed at
their original slots) remains unchanged, update any comments/struct layout
notes, and run the provided verification script to confirm slot ordering; ensure
no other reordering occurred for emissionsManager/baseMiningPass or related
fields.
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 177-201: _deployAndReplace currently only emits
FacetCutAction.Replace using loupe.facetFunctionSelectors(oldFacet), so any
selectors that are new in the upgraded DiamondManagerFacet (e.g.
getMiningPassTierFee, getMiningPassFeeFloorBps) never get registered and calls
revert; fix by computing the selector delta between the new facet's selector
list and the old facet's list (or use a hard-coded new-selector list) and append
a FacetCutAction.Add entry for that delta before making the cut array;
specifically update _deployAndReplace to call
loupe.facetFunctionSelectors(oldFacet), obtain newSelectors from the compiled
DiamondManagerFacet, diff newSelectors - oldSelectors (or use the hard-coded
list), and push a {facetAddress: newFacet, action: FacetCutAction.Add,
functionSelectors: delta} entry in addition to the existing Replace entry so the
new getters (getMiningPassTierFee, getMiningPassFeeFloorBps) are registered.
---
Nitpick comments:
In `@src/upgradeInitializers/FujiMigrationInit.sol`:
- Around line 63-93: The assembly cleanup block in FujiMigrationInit.sol
currently clears storage slots 27, 29, and 30 but skips slot 28 (commenting that
miningPassFeeFloorBps will be set later), leaving stratosphereAddress stale
until overwritten; add an sstore to explicitly zero slot 28 in the same assembly
cleanup sequence so slot 28 is cleared defensively along with 27/29/30 before
subsequent writes to miningPassFeeFloorBps or stratosphereAddress, referencing
the existing assembly block that performs sstore for slots 27, 29, and 30 and
the storage variables stratosphereAddress and miningPassFeeFloorBps.
- Around line 39-41: The init function in FujiMigrationInit.sol is missing a
one-time execution guard, so re-running the migration can silently overwrite
storage; add a guard at the top of init (e.g., a boolean
_fujiMigrationInitialized or check the new scalar miningPassFeeFloorBps) that
reverts if the migration already ran, and set that flag (or set
miningPassFeeFloorBps to its post-migration value like 2500) at the end of init;
reference the init function and the miningPassFeeFloorBps storage slot (or a new
private bool) to implement the check-and-set pattern to prevent re-execution.
- Around line 124-148: The assignments to miningPassTierToFee and
baseMiningPassTierToFee are duplicated; extract a single helper (e.g., internal
function initializeTierFees or _setTierFees) that accepts the tier keys and fee
values and sets both mappings in one loop, then call that helper from the
initializer instead of repeating the same value assignments so both mappings
remain in sync.
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 220-237: The catch in _ensureActiveSeason is too broad and may
swallow real post-migration failures from getSeasonEndTimestamp; change the
handler to only absorb the expected "no season" (or the exact revert
string/selector your contract emits) and rethrow any other errors—locate
_ensureActiveSeason and the call to getSeasonEndTimestamp and update the catch
to inspect the error (e.g., check error.message or error.selector for the known
"no season" marker) returning/starting a new season only when that specific
condition matches, otherwise propagate the error so unexpected ABI/storage/OOG
issues surface.
| // Unlock fee receivers (prod slots 9-10, old Fuji slots 12-13) | ||
| // Clear any stale length at slots 9-10 first | ||
| _clearArray(9); | ||
| _clearArray(10); | ||
| for (uint256 i = 0; i < _args.unlockFeeReceivers.length; i++) { | ||
| s.unlockFeeReceivers.push(_args.unlockFeeReceivers[i]); | ||
| s.unlockFeeReceiversShares.push(_args.unlockFeeReceiversShares[i]); | ||
| } | ||
|
|
||
| // Boost fee receivers (prod slots 14-15, old Fuji slots 17-18) | ||
| _clearArray(14); | ||
| _clearArray(15); | ||
| for (uint256 i = 0; i < _args.boostFeeReceivers.length; i++) { | ||
| s.boostFeeReceivers.push(_args.boostFeeReceivers[i]); | ||
| s.boostFeeReceiversShares.push(_args.boostFeeReceiversShares[i]); | ||
| } | ||
|
|
||
| // Mining pass fee receivers (prod slots 19-20, never existed in 14f06c0) | ||
| _clearArray(19); | ||
| _clearArray(20); | ||
| for (uint256 i = 0; i < _args.miningPassFeeReceivers.length; i++) { | ||
| s.miningPassFeeReceivers.push(_args.miningPassFeeReceivers[i]); | ||
| s.miningPassFeeReceiversShares.push(_args.miningPassFeeReceiversShares[i]); | ||
| } |
There was a problem hiding this comment.
Missing array-length validation between fee receivers and their shares.
All three loops iterate over _args.*FeeReceivers.length and index into the corresponding *FeeReceiversShares array at the same index. If shares.length < receivers.length, the transaction reverts with a generic Panic(0x32) (index out of bounds) instead of a descriptive error. If shares.length > receivers.length, the extra shares are silently dropped, producing an inconsistent configuration.
Add explicit length checks before each loop.
Proposed fix
+ require(
+ _args.unlockFeeReceivers.length == _args.unlockFeeReceiversShares.length,
+ "FujiMigration: unlock receivers/shares length mismatch"
+ );
// Unlock fee receivers (prod slots 9-10, old Fuji slots 12-13)
_clearArray(9);
_clearArray(10);
for (uint256 i = 0; i < _args.unlockFeeReceivers.length; i++) {
s.unlockFeeReceivers.push(_args.unlockFeeReceivers[i]);
s.unlockFeeReceiversShares.push(_args.unlockFeeReceiversShares[i]);
}
+ require(
+ _args.boostFeeReceivers.length == _args.boostFeeReceiversShares.length,
+ "FujiMigration: boost receivers/shares length mismatch"
+ );
// Boost fee receivers (prod slots 14-15, old Fuji slots 17-18)
_clearArray(14);
_clearArray(15);
for (uint256 i = 0; i < _args.boostFeeReceivers.length; i++) {
s.boostFeeReceivers.push(_args.boostFeeReceivers[i]);
s.boostFeeReceiversShares.push(_args.boostFeeReceiversShares[i]);
}
+ require(
+ _args.miningPassFeeReceivers.length == _args.miningPassFeeReceiversShares.length,
+ "FujiMigration: miningPass receivers/shares length mismatch"
+ );
// Mining pass fee receivers (prod slots 19-20, never existed in 14f06c0)
_clearArray(19);
_clearArray(20);
for (uint256 i = 0; i < _args.miningPassFeeReceivers.length; i++) {
s.miningPassFeeReceivers.push(_args.miningPassFeeReceivers[i]);
s.miningPassFeeReceiversShares.push(_args.miningPassFeeReceiversShares[i]);
}📝 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.
| // Unlock fee receivers (prod slots 9-10, old Fuji slots 12-13) | |
| // Clear any stale length at slots 9-10 first | |
| _clearArray(9); | |
| _clearArray(10); | |
| for (uint256 i = 0; i < _args.unlockFeeReceivers.length; i++) { | |
| s.unlockFeeReceivers.push(_args.unlockFeeReceivers[i]); | |
| s.unlockFeeReceiversShares.push(_args.unlockFeeReceiversShares[i]); | |
| } | |
| // Boost fee receivers (prod slots 14-15, old Fuji slots 17-18) | |
| _clearArray(14); | |
| _clearArray(15); | |
| for (uint256 i = 0; i < _args.boostFeeReceivers.length; i++) { | |
| s.boostFeeReceivers.push(_args.boostFeeReceivers[i]); | |
| s.boostFeeReceiversShares.push(_args.boostFeeReceiversShares[i]); | |
| } | |
| // Mining pass fee receivers (prod slots 19-20, never existed in 14f06c0) | |
| _clearArray(19); | |
| _clearArray(20); | |
| for (uint256 i = 0; i < _args.miningPassFeeReceivers.length; i++) { | |
| s.miningPassFeeReceivers.push(_args.miningPassFeeReceivers[i]); | |
| s.miningPassFeeReceiversShares.push(_args.miningPassFeeReceiversShares[i]); | |
| } | |
| require( | |
| _args.unlockFeeReceivers.length == _args.unlockFeeReceiversShares.length, | |
| "FujiMigration: unlock receivers/shares length mismatch" | |
| ); | |
| // Unlock fee receivers (prod slots 9-10, old Fuji slots 12-13) | |
| _clearArray(9); | |
| _clearArray(10); | |
| for (uint256 i = 0; i < _args.unlockFeeReceivers.length; i++) { | |
| s.unlockFeeReceivers.push(_args.unlockFeeReceivers[i]); | |
| s.unlockFeeReceiversShares.push(_args.unlockFeeReceiversShares[i]); | |
| } | |
| require( | |
| _args.boostFeeReceivers.length == _args.boostFeeReceiversShares.length, | |
| "FujiMigration: boost receivers/shares length mismatch" | |
| ); | |
| // Boost fee receivers (prod slots 14-15, old Fuji slots 17-18) | |
| _clearArray(14); | |
| _clearArray(15); | |
| for (uint256 i = 0; i < _args.boostFeeReceivers.length; i++) { | |
| s.boostFeeReceivers.push(_args.boostFeeReceivers[i]); | |
| s.boostFeeReceiversShares.push(_args.boostFeeReceiversShares[i]); | |
| } | |
| require( | |
| _args.miningPassFeeReceivers.length == _args.miningPassFeeReceiversShares.length, | |
| "FujiMigration: miningPass receivers/shares length mismatch" | |
| ); | |
| // Mining pass fee receivers (prod slots 19-20, never existed in 14f06c0) | |
| _clearArray(19); | |
| _clearArray(20); | |
| for (uint256 i = 0; i < _args.miningPassFeeReceivers.length; i++) { | |
| s.miningPassFeeReceivers.push(_args.miningPassFeeReceivers[i]); | |
| s.miningPassFeeReceiversShares.push(_args.miningPassFeeReceiversShares[i]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/upgradeInitializers/FujiMigrationInit.sol` around lines 99 - 122, The
three loops that iterate over _args.*FeeReceivers and index into the matching
*FeeReceiversShares arrays must validate that the receiver and share arrays have
identical lengths before looping; add explicit checks (e.g.,
require(_args.partnerFeeReceivers.length ==
_args.partnerFeeReceiversShares.length, "partner fee receivers/shares length
mismatch")) for each pair (partnerFeeReceivers vs partnerFeeReceiversShares,
platformFeeReceivers vs platformFeeReceiversShares, referralFeeReceivers vs
referralFeeReceiversShares) in the FujiMigrationInit migration function so you
revert with a clear error when lengths differ rather than allowing out-of-bounds
panics or silently dropping extra shares.
| function test_Fork_PreMigration_OldSlotLayout() public { | ||
| // Old Fuji 14f06c0 layout: GENERAL at slots 25-28 | ||
| address slot25 = _readAddress(25); | ||
| address slot26 = _readAddress(26); | ||
| address slot27 = _readAddress(27); | ||
| address slot28 = _readAddress(28); | ||
|
|
||
| console.log("Pre-migration (old Fuji slots 25-28):"); | ||
| console.log(" slot 25:", slot25); | ||
| console.log(" slot 26:", slot26); | ||
| console.log(" slot 27:", slot27); | ||
| console.log(" slot 28:", slot28); | ||
|
|
||
| assertEq(slot25, VPND, "old slot 25 should be VPND (depositToken)"); | ||
| assertEq(slot26, VAPE, "old slot 26 should be VAPE (rewardToken)"); | ||
| assertEq(slot27, USDC, "old slot 27 should be USDC (feeToken)"); | ||
| assertEq(slot28, STRATOSPHERE, "old slot 28 should be stratosphere"); | ||
|
|
||
| // Production slots 21-24 should NOT have these values yet | ||
| address slot21 = _readAddress(21); | ||
| address slot23 = _readAddress(23); | ||
| assertTrue(slot21 != VPND, "prod slot 21 should NOT have VPND yet"); | ||
| assertTrue(slot23 != USDC, "prod slot 23 should NOT have USDC yet"); | ||
| } |
There was a problem hiding this comment.
test_Fork_PreMigration_OldSlotLayout is permanently broken once the migration runs on Fuji.
vm.createSelectFork("fuji") without a block number always forks the current chain head. As soon as this PR's migration is applied to the Fuji testnet, slots 25–28 will no longer hold the old VPND/VAPE/USDC/STRATOSPHERE values, causing every assertion in this test to fail on every subsequent CI run.
Pin the fork to a block number captured before the migration to keep the pre-migration assertion stable:
- vm.createSelectFork("fuji");
+ vm.createSelectFork("fuji", PRE_MIGRATION_BLOCK); // e.g. uint256 constant PRE_MIGRATION_BLOCK = 12345678;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 64 - 87, The test
test_Fork_PreMigration_OldSlotLayout currently uses vm.createSelectFork("fuji")
which forks the current head and will break once the migration runs; update the
test to pin the fork to a specific block number captured before the migration by
calling vm.createSelectFork("fuji", PRE_MIGRATION_BLOCK) (replace
PRE_MIGRATION_BLOCK with the explicit block number taken from the chain before
the migration) so the assertions about slots 25–28 remain stable across CI runs.
| function _deployAndReplace(address newFacet, bytes4 knownSelector) | ||
| internal | ||
| view | ||
| returns (IDiamondCut.FacetCut memory) | ||
| { | ||
| address oldFacet = loupe.facetAddress(knownSelector); | ||
| bytes4[] memory selectors = loupe.facetFunctionSelectors(oldFacet); | ||
| return IDiamondCut.FacetCut({ | ||
| facetAddress: newFacet, action: IDiamondCut.FacetCutAction.Replace, functionSelectors: selectors | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing guard for address(0) returned by the loupe — results in a cryptic diamondCut revert.
If knownSelector is not registered on the current Fuji diamond, loupe.facetAddress(knownSelector) returns address(0), and loupe.facetFunctionSelectors(address(0)) returns an empty array. Passing an empty functionSelectors array to diamondCut violates EIP-2535 and will revert with "LibDiamondCut: No selectors in facet to cut" — with no indication of which facet/selector caused the problem.
🛡️ Proposed fix
function _deployAndReplace(address newFacet, bytes4 knownSelector)
internal
view
returns (IDiamondCut.FacetCut memory)
{
address oldFacet = loupe.facetAddress(knownSelector);
+ require(oldFacet != address(0), "_deployAndReplace: knownSelector not registered on diamond");
bytes4[] memory selectors = loupe.facetFunctionSelectors(oldFacet);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 191 - 201, The test
currently calls loupe.facetFunctionSelectors(loupe.facetAddress(knownSelector))
without checking for address(0), which causes diamondCut to fail cryptically if
knownSelector isn't registered; modify the test in FujiForkMiningPass.t.sol to
first call address facetAddr = loupe.facetAddress(knownSelector) and check
facetAddr != address(0) before calling loupe.facetFunctionSelectors, and if it's
address(0) either skip/assert-fail with a clear message identifying
knownSelector or throw a descriptive revert/log so you never pass an empty
selectors array into diamondCut.
- Updated UnlockFacet and WithdrawFacet deployments with new bytecode and addresses. - Added migration script for Fuji to handle contract upgrades and data migration. - Introduced task to update mining pass fees based on current VAPE price.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deployments/FeeCollectorFacet.json (1)
50-74:⚠️ Potential issue | 🟠 MajorStale
hardhatartifact — missingLAuthorizable__OnlyAuthorizederror and compiled with wrong Solidity version.The
hardhatartifact is out of sync withfujiandavalanchein two ways:
Missing error: The
LAuthorizable__OnlyAuthorizederror is present in bothfujiandavalancheABIs but absent from thehardhatABI (Lines 50–70). Thehardhatbytecode still uses the oldFeeCollectorFacet__Only_Ownerowner-slot check (7fc8fcad8db84d3cc18b4c41d551ea0ee66dd599cde068d998e57d5e09332c1320), whilefuji/avalancheuse the new role-basedLAuthorizable__OnlyAuthorizedrevert (0x2c52e074). Any unit tests exercising authorization against thehardhatartifact will test the old logic and silently miss authorization regressions in the updated contract.Solidity version mismatch: The
hardhatbytecode tail is64736f6c63430008110033(solc 0.8.17), whilefujiandavalancheare both64736f6c63430008120033(solc 0.8.18). This means thehardhatartifact was compiled from an older build.The
hardhatartifact should be regenerated from the same source that produced thefuji/avalancheartifacts to keep test coverage aligned with the deployed contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/FeeCollectorFacet.json` around lines 50 - 74, The hardhat artifact is stale: its ABI lacks the LAuthorizable__OnlyAuthorized error and still includes FeeCollectorFacet__Only_Owner, and its bytecode was compiled with solc 0.8.17; regenerate the artifact from the same source/compile config used for fuji/avalanche (so the ABI includes LAuthorizable__OnlyAuthorized and the bytecode tail matches solc 0.8.18) by re-running the build/compile step for FeeCollectorFacet and replacing the deployments/FeeCollectorFacet.json with the newly generated artifact.deployments/DepositFacet.json (1)
89-109:⚠️ Potential issue | 🟡 MinorHardhat ABI is missing the
DepositFacet__Less_Than_One_Dayerror present infujiandavalancheABIs.The source code (src/facets/DepositFacet.sol, line 16) defines this error, and both live-network deployment artifacts include it in their ABIs. The hardhat artifact appears to be stale—it contains only 4 custom errors while fuji and avalanche each have 5. Any test suite or decoding tool using the hardhat artifact will fail to recognize this error, creating an ABI mismatch.
Regenerate the hardhat artifact from the current source, or add the error to match:
➕ Proposed fix
{ "inputs": [], "name": "DepositFacet__InvalidMiningPass", "type": "error" }, + { + "inputs": [], + "name": "DepositFacet__Less_Than_One_Day", + "type": "error" + }, { "inputs": [], "name": "DepositFacet__NotEnoughTokenBalance",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/DepositFacet.json` around lines 89 - 109, Hardhat deployment artifact ABI for DepositFacet is missing the custom error DepositFacet__Less_Than_One_Day declared in src/facets/DepositFacet.sol; regenerate the Hardhat artifact or update the ABI so it includes the missing error entry (named DepositFacet__Less_Than_One_Day) alongside the other errors (e.g., DepositFacet__InvalidMiningPass, DepositFacet__NotEnoughTokenBalance, DepositFacet__ReentrancyGuard__ReentrantCall, DepositFacet__SeasonEnded) so the Hardhat artifact matches fuji/avalanche artifacts and decoding/tests will recognize the error.deployments/MiningPassFacet.json (1)
133-234:⚠️ Potential issue | 🟠 MajorHardhat artifact is stale — missing new ABI entries and using a different compiler version.
The
hardhatsection has two problems:
ABI drift:
MiningPassFacet__InvalidFeeReceiversandgetMiningPassFeeare both present infujiandavalanchebut absent from thehardhatartifact. Any tooling or script that reads the hardhat artifact from this file will silently miss these entries.Compiler version mismatch: The hardhat bytecode was compiled with Solidity
0.8.17(64736f6c63430008110033), while fuji and avalanche use0.8.18. All three should match to guarantee identical semantics and avoid surprises if behavior diverges between compiler patch releases.Regenerate the hardhat artifact with the same compiler version as the live networks after applying the same source changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/MiningPassFacet.json` around lines 133 - 234, The hardhat artifact is stale: update the hardhat artifact for MiningPassFacet so its ABI includes the missing entries (MiningPassFacet__InvalidFeeReceivers and getMiningPassFee) and recompile with the same compiler patch as fuji/avalanche (Solidity 0.8.18) so the deployedBytecode/hash matches; regenerate the Hardhat artifact under the "hardhat" section (the artifact for contractName "MiningPassFacet") by applying the same source changes, compiling with 0.8.18, and replacing the artifact’s abi and bytecode/deployedBytecode fields accordingly.
🧹 Nitpick comments (2)
scripts/migrateFuji.ts (1)
98-104: Log the transaction hash afteraddOrReplaceFacetsfor post-run auditability.The
diamondCuttransaction is the most critical on-chain action in this migration. Omitting the tx hash in the console output makes it impossible to quickly verify the migration on-chain (e.g., on Snowtrace) without querying the deployer's tx history separately.🔭 Suggested improvement
- await addOrReplaceFacets( + const tx = await addOrReplaceFacets( Facets, diamondAddress, migrationInitAddress, initCalldata, undefined // no Safe — direct EOA tx ) + console.log(` diamondCut tx hash: ${tx?.hash ?? 'unknown'}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrateFuji.ts` around lines 98 - 104, After awaiting addOrReplaceFacets(Facets, diamondAddress, migrationInitAddress, initCalldata, undefined), capture the returned transaction object and log its hash for auditability; update the call site in scripts/migrateFuji.ts (around the addOrReplaceFacets invocation) to assign the result to a variable (e.g., tx or txReceipt) and add a console.log that prints a clear message including the transaction hash (tx.hash) so the diamondCut can be easily verified on-chain.tasks/updateMiningPassFees.ts (1)
99-103: Skip on-chain write when computed fees are unchanged.You already read
currentFees; use it to avoid no-op transactions.Suggested fix
// ── 5. Update mining pass fees ─────────────────────────────────── + const hasChanges = dynamicFees.some((fee, i) => fee !== currentFees[i]) + if (!hasChanges) { + console.log('\n✅ Fees already up to date — skipping transaction.') + return + } + console.log('\n⏳ Updating mining pass fees...') const feesTx = await DiamondManagerFacet.setMiningPassFees(dynamicFees) await logTx(feesTx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/updateMiningPassFees.ts` around lines 99 - 103, Compare the computed dynamicFees with the already-read currentFees and skip calling DiamondManagerFacet.setMiningPassFees/logTx when they are equal: compute an equality check between currentFees and dynamicFees (deep/equivalent comparison for arrays/objects) and only call const feesTx = await DiamondManagerFacet.setMiningPassFees(dynamicFees) followed by await logTx(feesTx) if they differ; otherwise print a message like "fees unchanged, skipping on-chain update" to avoid a no-op transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/migrateFuji.ts`:
- Around line 57-60: The facet deployments currently call deployContract(name)
which defaults to skipIfAlreadyDeployed: true; change the loop that builds
Facets (using FacetNames and deployContract) to explicitly pass {
skipIfAlreadyDeployed: false } so each facet is always redeployed (matching the
explicit behavior used for FujiMigrationInit) and avoid reusing stale bytecode
when updating diamond logic.
In `@tasks/updateMiningPassFees.ts`:
- Around line 40-42: The lookup of LiquidMiningDiamond using network.name can
return undefined and cause a TypeError; add an explicit guard before computing
diamondAddress: verify that LiquidMiningDiamond has a mapping for network.name
(e.g., check hasOwnProperty or that the lookup result is truthy) and if missing
throw a clear Error mentioning the missing deployment mapping and the active
network name. Update the logic around the existing
LiquidMiningDiamond[network.name as keyof typeof LiquidMiningDiamond] access
(used to compute diamondAddress) to perform this check and fail fast with a
descriptive message so callers know to add the network entry.
- Around line 50-52: The code assigns currentVAPEPrice from vapePrice via
parseFloat and later converts values to BigInt which throws on invalid input;
validate vapePrice before doing parseFloat/BigInt by checking that vapePrice is
defined, parseFloat(vapePrice) yields a finite number and >0, and only then use
it for currentVAPEPrice (otherwise call fetchVAPEPrice() or throw a clear CLI
error); update places that perform BigInt conversion to guard/short-circuit if
currentVAPEPrice is invalid (references: the currentVAPEPrice variable
assignment, the vapePrice input, fetchVAPEPrice(), and the subsequent
BigInt(...) conversions) so the task fails with a descriptive message instead of
a generic runtime error.
- Around line 9-16: The fetchVAPEPrice function should be hardened: add an
AbortController timeout around the fetch and check response.ok before parsing;
if response.ok is false or the request times out or parsing fails, log or
capture the error and return a sensible fallback/default price (matching the
pattern in scripts/simulation/miningPassPricing.ts) instead of throwing; update
fetchVAPEPrice to use the AbortController timeout, verify HTTP status, handle
JSON parsing errors, and return the default price on any failure.
---
Outside diff comments:
In `@deployments/DepositFacet.json`:
- Around line 89-109: Hardhat deployment artifact ABI for DepositFacet is
missing the custom error DepositFacet__Less_Than_One_Day declared in
src/facets/DepositFacet.sol; regenerate the Hardhat artifact or update the ABI
so it includes the missing error entry (named DepositFacet__Less_Than_One_Day)
alongside the other errors (e.g., DepositFacet__InvalidMiningPass,
DepositFacet__NotEnoughTokenBalance,
DepositFacet__ReentrancyGuard__ReentrantCall, DepositFacet__SeasonEnded) so the
Hardhat artifact matches fuji/avalanche artifacts and decoding/tests will
recognize the error.
In `@deployments/FeeCollectorFacet.json`:
- Around line 50-74: The hardhat artifact is stale: its ABI lacks the
LAuthorizable__OnlyAuthorized error and still includes
FeeCollectorFacet__Only_Owner, and its bytecode was compiled with solc 0.8.17;
regenerate the artifact from the same source/compile config used for
fuji/avalanche (so the ABI includes LAuthorizable__OnlyAuthorized and the
bytecode tail matches solc 0.8.18) by re-running the build/compile step for
FeeCollectorFacet and replacing the deployments/FeeCollectorFacet.json with the
newly generated artifact.
In `@deployments/MiningPassFacet.json`:
- Around line 133-234: The hardhat artifact is stale: update the hardhat
artifact for MiningPassFacet so its ABI includes the missing entries
(MiningPassFacet__InvalidFeeReceivers and getMiningPassFee) and recompile with
the same compiler patch as fuji/avalanche (Solidity 0.8.18) so the
deployedBytecode/hash matches; regenerate the Hardhat artifact under the
"hardhat" section (the artifact for contractName "MiningPassFacet") by applying
the same source changes, compiling with 0.8.18, and replacing the artifact’s abi
and bytecode/deployedBytecode fields accordingly.
---
Nitpick comments:
In `@scripts/migrateFuji.ts`:
- Around line 98-104: After awaiting addOrReplaceFacets(Facets, diamondAddress,
migrationInitAddress, initCalldata, undefined), capture the returned transaction
object and log its hash for auditability; update the call site in
scripts/migrateFuji.ts (around the addOrReplaceFacets invocation) to assign the
result to a variable (e.g., tx or txReceipt) and add a console.log that prints a
clear message including the transaction hash (tx.hash) so the diamondCut can be
easily verified on-chain.
In `@tasks/updateMiningPassFees.ts`:
- Around line 99-103: Compare the computed dynamicFees with the already-read
currentFees and skip calling DiamondManagerFacet.setMiningPassFees/logTx when
they are equal: compute an equality check between currentFees and dynamicFees
(deep/equivalent comparison for arrays/objects) and only call const feesTx =
await DiamondManagerFacet.setMiningPassFees(dynamicFees) followed by await
logTx(feesTx) if they differ; otherwise print a message like "fees unchanged,
skipping on-chain update" to avoid a no-op transaction.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
deployments/AuthorizationFacet.jsondeployments/BoostFacet.jsondeployments/ClaimFacet.jsondeployments/DepositFacet.jsondeployments/DiamondManagerFacet.jsondeployments/FeeCollectorFacet.jsondeployments/FujiMigrationInit.jsondeployments/MiningPassFacet.jsondeployments/PausationFacet.jsondeployments/UnlockFacet.jsondeployments/WithdrawFacet.jsonhardhat.config.tsscripts/migrateFuji.tstasks/updateMiningPassFees.ts
✅ Files skipped from review due to trivial changes (1)
- deployments/FujiMigrationInit.json
🚧 Files skipped from review as they are similar to previous changes (6)
- hardhat.config.ts
- deployments/UnlockFacet.json
- deployments/WithdrawFacet.json
- deployments/PausationFacet.json
- deployments/BoostFacet.json
- deployments/ClaimFacet.json
| const Facets = [] | ||
| for (const name of FacetNames) { | ||
| Facets.push(await deployContract(name)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd deployContract -type f --extension ts --extension js | head -20Repository: VaporFi/liquid-mining
Length of output: 235
🏁 Script executed:
fd deployContract -type fRepository: VaporFi/liquid-mining
Length of output: 235
🏁 Script executed:
fd deployContract --type fRepository: VaporFi/liquid-mining
Length of output: 88
🏁 Script executed:
cat -n utils/deployContract.tsRepository: VaporFi/liquid-mining
Length of output: 3033
🏁 Script executed:
cat -n scripts/migrateFuji.tsRepository: VaporFi/liquid-mining
Length of output: 5351
Set skipIfAlreadyDeployed: false for facet deployments to ensure consistent migration behavior.
FujiMigrationInit is explicitly deployed with skipIfAlreadyDeployed: false (line 47), but facets are deployed without any options. The deployContract utility defaults to skipIfAlreadyDeployed: true, which means this migration will silently reuse previously-deployed facet artifacts instead of deploying fresh ones. For a migration that specifically updates facet logic, this creates a dangerous behavioral inconsistency where stale bytecode could be wired into the diamond.
🐛 Proposed fix
- const Facets = []
- for (const name of FacetNames) {
- Facets.push(await deployContract(name))
- }
+ const Facets = []
+ for (const name of FacetNames) {
+ Facets.push(await deployContract(name, { skipIfAlreadyDeployed: false }))
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/migrateFuji.ts` around lines 57 - 60, The facet deployments currently
call deployContract(name) which defaults to skipIfAlreadyDeployed: true; change
the loop that builds Facets (using FacetNames and deployContract) to explicitly
pass { skipIfAlreadyDeployed: false } so each facet is always redeployed
(matching the explicit behavior used for FujiMigrationInit) and avoid reusing
stale bytecode when updating diamond logic.
| const diamondAddress = | ||
| LiquidMiningDiamond[network.name as keyof typeof LiquidMiningDiamond] | ||
| .address |
There was a problem hiding this comment.
Guard missing deployment mapping for the active network.
If network.name is absent in LiquidMiningDiamond.json, this throws a generic TypeError instead of a clear config error.
Suggested fix
- const diamondAddress =
- LiquidMiningDiamond[network.name as keyof typeof LiquidMiningDiamond]
- .address
+ const deployment =
+ LiquidMiningDiamond[
+ network.name as keyof typeof LiquidMiningDiamond
+ ]
+ if (!deployment?.address) {
+ throw new Error(
+ `No LiquidMiningDiamond deployment found for network "${network.name}"`
+ )
+ }
+ const diamondAddress = deployment.address📝 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.
| const diamondAddress = | |
| LiquidMiningDiamond[network.name as keyof typeof LiquidMiningDiamond] | |
| .address | |
| const deployment = | |
| LiquidMiningDiamond[ | |
| network.name as keyof typeof LiquidMiningDiamond | |
| ] | |
| if (!deployment?.address) { | |
| throw new Error( | |
| `No LiquidMiningDiamond deployment found for network "${network.name}"` | |
| ) | |
| } | |
| const diamondAddress = deployment.address |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/updateMiningPassFees.ts` around lines 40 - 42, The lookup of
LiquidMiningDiamond using network.name can return undefined and cause a
TypeError; add an explicit guard before computing diamondAddress: verify that
LiquidMiningDiamond has a mapping for network.name (e.g., check hasOwnProperty
or that the lookup result is truthy) and if missing throw a clear Error
mentioning the missing deployment mapping and the active network name. Update
the logic around the existing LiquidMiningDiamond[network.name as keyof typeof
LiquidMiningDiamond] access (used to compute diamondAddress) to perform this
check and fail fast with a descriptive message so callers know to add the
network entry.
| const currentVAPEPrice = vapePrice | ||
| ? parseFloat(vapePrice) | ||
| : await fetchVAPEPrice() |
There was a problem hiding this comment.
Validate vapePrice input before fee math.
Invalid CLI input currently propagates to BigInt(...) conversion and fails with a non-actionable runtime error.
Suggested fix
const currentVAPEPrice = vapePrice
- ? parseFloat(vapePrice)
+ ? Number(vapePrice)
: await fetchVAPEPrice()
+ if (!Number.isFinite(currentVAPEPrice) || currentVAPEPrice <= 0) {
+ throw new Error(
+ `Invalid vapePrice "${vapePrice}". Provide a positive numeric USD value.`
+ )
+ }📝 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.
| const currentVAPEPrice = vapePrice | |
| ? parseFloat(vapePrice) | |
| : await fetchVAPEPrice() | |
| const currentVAPEPrice = vapePrice | |
| ? Number(vapePrice) | |
| : await fetchVAPEPrice() | |
| if (!Number.isFinite(currentVAPEPrice) || currentVAPEPrice <= 0) { | |
| throw new Error( | |
| `Invalid vapePrice "${vapePrice}". Provide a positive numeric USD value.` | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/updateMiningPassFees.ts` around lines 50 - 52, The code assigns
currentVAPEPrice from vapePrice via parseFloat and later converts values to
BigInt which throws on invalid input; validate vapePrice before doing
parseFloat/BigInt by checking that vapePrice is defined, parseFloat(vapePrice)
yields a finite number and >0, and only then use it for currentVAPEPrice
(otherwise call fetchVAPEPrice() or throw a clear CLI error); update places that
perform BigInt conversion to guard/short-circuit if currentVAPEPrice is invalid
(references: the currentVAPEPrice variable assignment, the vapePrice input,
fetchVAPEPrice(), and the subsequent BigInt(...) conversions) so the task fails
with a descriptive message instead of a generic runtime error.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/foundry/fork/FujiForkMiningPass.t.sol (2)
115-116:deal(VPND, …, fee * 10)ties VPND amount to a USDC-denominated fee — different decimals may be misleading.USDC is typically 6-decimal while VPND may be 18-decimal, so
fee * 10yields a very different real-world amount for each token. Since the only purpose is to verifyvpndSpent == 0, consider using a fixed human-readable amount (e.g.,1000 * 1e18) for the VPND deal to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 115 - 116, The test uses deal(USDC, user, fee * 10) and deal(VPND, user, fee * 10) which ties VPND to a USDC-denominated fee and is misleading due to different decimals; change the VPND allocation to a fixed human-readable amount (e.g., 1000 * 1e18) instead of fee * 10 so the VPND balance is clearly sufficient for the test and unrelated to USDC decimals, and ensure any assertions referencing vpndSpent or VPND balance use the new fixed amount.
46-60: NatSpec says "slots 21-26" but the test checks slots 21–25 and 28 (skipping 26–27).The comment on line 46 and the contract-level doc on line 13 both reference "slots 21-26", but the actual assertions cover slots 21–24, 25, and 28. Either add assertions for slots 26 and 27 or update the comments to reflect the actual range (e.g., "slots 21–25, 28").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 46 - 60, Update the comment/NatSpec to match the actual assertions in test_Fork_StorageSlots_GeneralSection: change the "slots 21-26" wording to "slots 21–25, 28" (or alternatively add assertions that read slots 26 and 27 using _readAddress or vm.load against DIAMOND if those slots should be checked); ensure any inline comment near the uint256 reads (vm.load(DIAMOND, bytes32(uint256(25/28)))) and the function-level doc are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 29-40: The test forks the Fuji network at its latest block in
setUp using vm.createSelectFork("fuji"), which makes tests non-deterministic;
change setUp to pin the fork to a fixed block by calling the two-arg
createSelectFork overload (vm.createSelectFork("fuji", blockNumber)) instead of
the single-arg version and provide that block number via a constant or env var
(e.g., FUJI_FORK_BLOCK or vm.envUint("FUJI_FORK_BLOCK")) so the forked state is
stable for assertions in setUp (references: setUp function and
vm.createSelectFork).
- Around line 145-162: The call in _ensureActiveSeason to
diamond.startNewSeasonWithEndTimestamp(...) can revert if owner lacks VAPE
balance/allowance; before any owner prank that starts a season, ensure the owner
has >= 1000e18 VAPE and has approved the diamond contract: credit the owner's
VAPE balance (e.g., mint/transfer the token to owner or use the test cheat to
set ERC20 balance) and then vm.prank(owner) to call
IERC20(vape).approve(address(diamond), 1000 * 1e18) (or equivalent) prior to
calling diamond.startNewSeasonWithEndTimestamp in the branches inside
_ensureActiveSeason.
---
Nitpick comments:
In `@test/foundry/fork/FujiForkMiningPass.t.sol`:
- Around line 115-116: The test uses deal(USDC, user, fee * 10) and deal(VPND,
user, fee * 10) which ties VPND to a USDC-denominated fee and is misleading due
to different decimals; change the VPND allocation to a fixed human-readable
amount (e.g., 1000 * 1e18) instead of fee * 10 so the VPND balance is clearly
sufficient for the test and unrelated to USDC decimals, and ensure any
assertions referencing vpndSpent or VPND balance use the new fixed amount.
- Around line 46-60: Update the comment/NatSpec to match the actual assertions
in test_Fork_StorageSlots_GeneralSection: change the "slots 21-26" wording to
"slots 21–25, 28" (or alternatively add assertions that read slots 26 and 27
using _readAddress or vm.load against DIAMOND if those slots should be checked);
ensure any inline comment near the uint256 reads (vm.load(DIAMOND,
bytes32(uint256(25/28)))) and the function-level doc are consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/foundry/fork/FujiForkMiningPass.t.sol
| function setUp() public { | ||
| vm.createSelectFork("fuji"); | ||
|
|
||
| diamond = DiamondManagerFacet(DIAMOND); | ||
| miningPass = MiningPassFacet(DIAMOND); | ||
| loupe = IDiamondLoupe(DIAMOND); | ||
| user = makeAddr("testUser"); | ||
|
|
||
| (bool ok, bytes memory data) = DIAMOND.staticcall(abi.encodeWithSignature("owner()")); | ||
| require(ok, "owner() call failed"); | ||
| owner = abi.decode(data, (address)); | ||
| } |
There was a problem hiding this comment.
Pin the fork to a specific post-migration block number to make tests deterministic.
vm.createSelectFork("fuji") forks at the latest block, so every test assertion (floorBps == 2500, tier fees, slot contents, season state…) can silently break whenever the on-chain state drifts — fee updates, new seasons, storage-slot changes, etc. This also makes CI results non-reproducible.
Proposed fix
+ // Block captured right after the migration tx on Fuji.
+ uint256 constant FORK_BLOCK = 12_345_678; // ← replace with actual block
+
function setUp() public {
- vm.createSelectFork("fuji");
+ vm.createSelectFork("fuji", FORK_BLOCK);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 29 - 40, The test
forks the Fuji network at its latest block in setUp using
vm.createSelectFork("fuji"), which makes tests non-deterministic; change setUp
to pin the fork to a fixed block by calling the two-arg createSelectFork
overload (vm.createSelectFork("fuji", blockNumber)) instead of the single-arg
version and provide that block number via a constant or env var (e.g.,
FUJI_FORK_BLOCK or vm.envUint("FUJI_FORK_BLOCK")) so the forked state is stable
for assertions in setUp (references: setUp function and vm.createSelectFork).
| function _ensureActiveSeason() internal { | ||
| uint256 seasonId = diamond.getCurrentSeasonId(); | ||
| if (seasonId == 0) { | ||
| vm.prank(owner); | ||
| diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days); | ||
| return; | ||
| } | ||
|
|
||
| try diamond.getSeasonEndTimestamp(seasonId) returns (uint256 endTs) { | ||
| if (endTs <= block.timestamp) { | ||
| vm.prank(owner); | ||
| diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days); | ||
| } | ||
| } catch { | ||
| vm.prank(owner); | ||
| diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days); | ||
| } | ||
| } |
There was a problem hiding this comment.
_ensureActiveSeason may revert if the owner lacks VAPE balance for the reward deposit.
startNewSeasonWithEndTimestamp(1000 * 1e18, …) will likely transferFrom the owner's VAPE. On a live fork the owner's token balance is whatever it happens to be at the forked block — if it's below 1000e18 or lacks an existing approval, the call reverts with a generic ERC20 error.
Consider adding deal + approval before starting the season:
Proposed hardening
+ function _fundAndApproveOwner() internal {
+ deal(VAPE, owner, 10_000 * 1e18);
+ vm.prank(owner);
+ IERC20(VAPE).approve(DIAMOND, type(uint256).max);
+ }
+
function _ensureActiveSeason() internal {
uint256 seasonId = diamond.getCurrentSeasonId();
if (seasonId == 0) {
+ _fundAndApproveOwner();
vm.prank(owner);
diamond.startNewSeasonWithEndTimestamp(1000 * 1e18, block.timestamp + 30 days);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/foundry/fork/FujiForkMiningPass.t.sol` around lines 145 - 162, The call
in _ensureActiveSeason to diamond.startNewSeasonWithEndTimestamp(...) can revert
if owner lacks VAPE balance/allowance; before any owner prank that starts a
season, ensure the owner has >= 1000e18 VAPE and has approved the diamond
contract: credit the owner's VAPE balance (e.g., mint/transfer the token to
owner or use the test cheat to set ERC20 balance) and then vm.prank(owner) to
call IERC20(vape).approve(address(diamond), 1000 * 1e18) (or equivalent) prior
to calling diamond.startNewSeasonWithEndTimestamp in the branches inside
_ensureActiveSeason.
2 participants
Summary by CodeRabbit
New Features
Analytics
Tests
Chores