feat: refactor mining pass fee tasks and replace updateMiningPassFees… - #66
Merged
Conversation
… with miningPassFees
📝 WalkthroughWalkthroughConsolidates mining-pass fee tooling: deletes the old single-task script, adds a new multi-task Hardhat module for reading/updating/init-setting mining pass fees (including VAPE price fetch and dynamic fee calculation), updates hardhat.config import, and adds two post-upgrade foundry tests verifying storage integrity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Task as Hardhat Task\n(mining-pass:update-fees)
participant CG as CoinGecko API
participant DM as DiamondManagerFacet
User->>Task: run task (optional vapePrice, dryRun flag)
alt vapePrice not provided
Task->>CG: fetch current VAPE price
CG-->>Task: return price
end
Task->>DM: read baseMiningPassTierFees (11 tiers)
DM-->>Task: return base fees
Task->>DM: read currentMiningPassTierFees (11 tiers)
DM-->>Task: return current fees
Task->>Task: calculateDynamicFees(baseFees, vapePrice, floorBps)
Task-->>User: display comparison table (Base | Current | New | %)
alt dryRun = false
Task->>DM: setMiningPassFees(dynamicFees)
DM-->>Task: tx receipt
Task-->>User: log tx hash
else dryRun = true
Task-->>User: simulation complete (no tx)
end
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: 3
🧹 Nitpick comments (2)
tasks/miningPassFees.ts (2)
35-38: Add an explicit guard for missing deployment addresses.If the current network is not present in
LiquidMiningDiamond.json, this path throws a generic property access error. A clear task-level error improves operator diagnostics.Suggested refactor
function getDiamondAddress(networkName: string): string { - return LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond] - .address + const deployment = + LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond] + if (!deployment?.address) { + throw new Error(`No Diamond deployment found for network: ${networkName}`) + } + return deployment.address }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/miningPassFees.ts` around lines 35 - 38, The getDiamondAddress function currently assumes LiquidMiningDiamond[networkName] exists; add an explicit guard that checks the lookup (LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond]) and its .address before returning, and if missing throw a clear Error (e.g. "Missing LiquidMiningDiamond deployment for network: <networkName>") so operators get a descriptive failure instead of a generic property access error; update getDiamondAddress to validate the entry and .address and throw that descriptive error when absent.
333-334: Reuse the shared floor constant in init path.Line 334 hardcodes
2500whileFLOOR_BPSalready exists. This can drift if the default changes.Suggested refactor
- console.log('⏳ Setting mining pass fee floor to 2500 bps (25%)...') - const floorTx = await manager.setMiningPassFeeFloor(2500) + console.log(`⏳ Setting mining pass fee floor to ${FLOOR_BPS} bps (${(FLOOR_BPS / 100).toFixed(1)}%)...`) + const floorTx = await manager.setMiningPassFeeFloor(FLOOR_BPS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/miningPassFees.ts` around lines 333 - 334, Replace the hardcoded 2500 with the shared constant FLOOR_BPS when calling manager.setMiningPassFeeFloor and when composing the console log; i.e., update the line using manager.setMiningPassFeeFloor(2500) to use FLOOR_BPS and change the message string ('⏳ Setting mining pass fee floor to 2500 bps (25%)...') to reference FLOOR_BPS so the init path reuses the canonical constant (ensure FLOOR_BPS is in scope/imports if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tasks/miningPassFees.ts`:
- Around line 129-131: currentVAPEPrice is assigned from vapePrice without
validating it, which can pass NaN/<=0 into calculateDynamicFees and break BigInt
conversions; update the logic that sets currentVAPEPrice (the vapePrice parse
branch where vapePrice ? parseFloat(vapePrice) : await fetchVAPEPrice()) to
validate the parsed value is a finite number > 0 (Number.isFinite and >0) and if
invalid fall back to await fetchVAPEPrice() or throw a clear error, then only
pass the validated currentVAPEPrice into calculateDynamicFees.
- Around line 77-79: The two console.log lines that print fee labels are
swapped: the line labeling "Base Fee" currently prints currentFee and the line
labeling "Current Fee" prints baseFee; update the output so "Base Fee" prints
baseFee and "Current Fee" prints currentFee (leave the "Floor Fee" line using
floorFee unchanged) by swapping the variables used in the corresponding
console.log calls where currentFee, baseFee, and floorFee are formatted.
- Around line 153-157: The code is using the hardcoded FLOOR_BPS when computing
dynamicFees; instead read the current on-chain floor and pass that into
calculateDynamicFees rather than FLOOR_BPS. Locate where you build fees in
mining-pass:update-fees, fetch the contract floor (e.g., call the mining pass
contract's floor/ floorBps/getFloorBps method via the existing contract
instance), parse it to the expected number, and use that value in the
calculateDynamicFees(...) call (with a safe fallback to the previous default
only if the on-chain read fails). Ensure the variable names dynamicFees,
calculateDynamicFees, and FLOOR_BPS are updated accordingly so the on-chain
floor is used.
---
Nitpick comments:
In `@tasks/miningPassFees.ts`:
- Around line 35-38: The getDiamondAddress function currently assumes
LiquidMiningDiamond[networkName] exists; add an explicit guard that checks the
lookup (LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond])
and its .address before returning, and if missing throw a clear Error (e.g.
"Missing LiquidMiningDiamond deployment for network: <networkName>") so
operators get a descriptive failure instead of a generic property access error;
update getDiamondAddress to validate the entry and .address and throw that
descriptive error when absent.
- Around line 333-334: Replace the hardcoded 2500 with the shared constant
FLOOR_BPS when calling manager.setMiningPassFeeFloor and when composing the
console log; i.e., update the line using manager.setMiningPassFeeFloor(2500) to
use FLOOR_BPS and change the message string ('⏳ Setting mining pass fee floor to
2500 bps (25%)...') to reference FLOOR_BPS so the init path reuses the canonical
constant (ensure FLOOR_BPS is in scope/imports if necessary).
ℹ️ 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 (4)
hardhat.config.tstasks/miningPassFees.tstasks/updateMiningPassFees.tstest/foundry/fork/ProdForkUpgrade.t.sol
💤 Files with no reviewable changes (1)
- tasks/updateMiningPassFees.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tasks/miningPassFees.ts (1)
221-233: Extract duplicated base-fee ladder into a shared constant.
BASE_FEESis duplicated in two tasks. Centralizing it avoids drift betweenmining-pass:set-base-feesandmining-pass:init.Also applies to: 304-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/miningPassFees.ts` around lines 221 - 233, There are two duplicated BASE_FEES definitions used by the mining-pass:set-base-fees and mining-pass:init tasks; extract a single exported constant (e.g., export const BASE_FEES: bigint[] = [...]) into a shared module and replace the local BASE_FEES arrays in both task files with an import from that shared module, updating any references to use the imported symbol and removing the duplicate definitions; ensure the constant preserves the bigint literals (0n, 500_000n, etc.) and update imports/exports, then run the build/tests to confirm no missing imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tasks/miningPassFees.ts`:
- Around line 35-38: The getDiamondAddress function currently does a blind
property access on LiquidMiningDiamond and will throw an opaque error if the
network key is missing; update getDiamondAddress (the function named
getDiamondAddress and the LiquidMiningDiamond lookup) to first validate that
LiquidMiningDiamond has an entry for networkName (e.g., via hasOwnProperty or by
checking the lookup result !== undefined), and if not, throw a clear,
fast-failing Error like "Unknown network for LiquidMiningDiamond: <networkName>"
so callers get an explicit message instead of a property-access exception; if
present, return the .address as before.
- Around line 163-167: The code treats onChainFloorBps === 0 as “unset” by using
onChainFloorBps > 0, so explicit zero values are ignored; change the check to
detect absence instead (e.g., onChainFloorBps !== undefined && onChainFloorBps
!== null && Number.isFinite(onChainFloorBps)) and use that to pick
effectiveFloorBps (effectiveFloorBps = hasOnChain ? onChainFloorBps :
FLOOR_BPS), and update the console.log condition to show the “[fallback —
on-chain not set]” only when onChainFloorBps is actually missing (not when it is
0). Use the existing symbols effectiveFloorBps, onChainFloorBps, FLOOR_BPS and
the console.log call to locate and update the logic.
---
Nitpick comments:
In `@tasks/miningPassFees.ts`:
- Around line 221-233: There are two duplicated BASE_FEES definitions used by
the mining-pass:set-base-fees and mining-pass:init tasks; extract a single
exported constant (e.g., export const BASE_FEES: bigint[] = [...]) into a shared
module and replace the local BASE_FEES arrays in both task files with an import
from that shared module, updating any references to use the imported symbol and
removing the duplicate definitions; ensure the constant preserves the bigint
literals (0n, 500_000n, etc.) and update imports/exports, then run the
build/tests to confirm no missing imports.
ℹ️ 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)
tasks/miningPassFees.ts
Comment on lines
+35
to
+38
| function getDiamondAddress(networkName: string): string { | ||
| return LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond] | ||
| .address | ||
| } |
There was a problem hiding this comment.
Add explicit deployment lookup validation for unknown networks.
At Line 35-38, missing network entries in LiquidMiningDiamond.json will throw an opaque property-access error. Fail fast with a clear message.
Suggested fix
function getDiamondAddress(networkName: string): string {
- return LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond]
- .address
+ const deployment =
+ LiquidMiningDiamond[networkName as keyof typeof LiquidMiningDiamond]
+ if (!deployment?.address) {
+ throw new Error(
+ `No LiquidMiningDiamond deployment found for network "${networkName}"`
+ )
+ }
+ return deployment.address
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/miningPassFees.ts` around lines 35 - 38, The getDiamondAddress function
currently does a blind property access on LiquidMiningDiamond and will throw an
opaque error if the network key is missing; update getDiamondAddress (the
function named getDiamondAddress and the LiquidMiningDiamond lookup) to first
validate that LiquidMiningDiamond has an entry for networkName (e.g., via
hasOwnProperty or by checking the lookup result !== undefined), and if not,
throw a clear, fast-failing Error like "Unknown network for LiquidMiningDiamond:
<networkName>" so callers get an explicit message instead of a property-access
exception; if present, return the .address as before.
Comment on lines
+163
to
+167
| const effectiveFloorBps = | ||
| onChainFloorBps > 0 ? onChainFloorBps : FLOOR_BPS | ||
| console.log( | ||
| `🛡️ Floor: ${effectiveFloorBps} bps (${(effectiveFloorBps / 100).toFixed(1)}%)${onChainFloorBps === 0 ? ' [fallback — on-chain not set]' : ''}` | ||
| ) |
There was a problem hiding this comment.
Honor explicit on-chain floor values (including 0 bps).
At Line 163, onChainFloorBps > 0 ? onChainFloorBps : FLOOR_BPS treats 0 as “unset,” so mining-pass:set-floor --bps 0 can never be respected during fee updates.
Suggested fix
- const onChainFloorBps = Number(await manager.getMiningPassFeeFloorBps())
- const effectiveFloorBps =
- onChainFloorBps > 0 ? onChainFloorBps : FLOOR_BPS
+ const onChainFloorBps = Number(await manager.getMiningPassFeeFloorBps())
+ if (
+ !Number.isFinite(onChainFloorBps) ||
+ onChainFloorBps < 0 ||
+ onChainFloorBps > 10000
+ ) {
+ throw new Error(`Invalid on-chain floor bps: ${onChainFloorBps}`)
+ }
+ const effectiveFloorBps = onChainFloorBps
console.log(
- `🛡️ Floor: ${effectiveFloorBps} bps (${(effectiveFloorBps / 100).toFixed(1)}%)${onChainFloorBps === 0 ? ' [fallback — on-chain not set]' : ''}`
+ `🛡️ Floor: ${effectiveFloorBps} bps (${(effectiveFloorBps / 100).toFixed(1)}%)`
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/miningPassFees.ts` around lines 163 - 167, The code treats
onChainFloorBps === 0 as “unset” by using onChainFloorBps > 0, so explicit zero
values are ignored; change the check to detect absence instead (e.g.,
onChainFloorBps !== undefined && onChainFloorBps !== null &&
Number.isFinite(onChainFloorBps)) and use that to pick effectiveFloorBps
(effectiveFloorBps = hasOnChain ? onChainFloorBps : FLOOR_BPS), and update the
console.log condition to show the “[fallback — on-chain not set]” only when
onChainFloorBps is actually missing (not when it is 0). Use the existing symbols
effectiveFloorBps, onChainFloorBps, FLOOR_BPS and the console.log call to locate
and update the logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… with miningPassFees
Summary by CodeRabbit
New Features
Tests