Fees deposit hook <-> Solana - DO NOT MERGE #206
Conversation
WalkthroughAdded a rescue function to Credit for owner-queued fee withdrawals; added HyperEVM (chainSlug 999) gasLimit override in WritePrecompile; introduced FeesPlugV2 and many deployment/verification/address/config updates; adjusted chain gas overrides and native token mapping. Changes
Sequence Diagram(s)Feature: Credit.rescue enqueues a withdrawal request sequenceDiagram
participant Owner as Owner (EOA)
participant Credit as Credit Contract
participant Router as Internal Request System
participant FeesPlug as IFeesPlug (target)
Owner->>Credit: call rescue(chainSlug, token, amount, maxFees, receiver)
Note right of Credit: owner check -> build payload\nwithdrawFees(token, receiver, amount)
Credit->>Router: _createRequest(consumeFrom=Credit, chainSlug, payload, maxFees)
Note right of Router: request queued for off-chain relayer\n(or precompile execution)
Router->>FeesPlug: (eventual) execute payload -> withdrawFees(...)
Feature: WritePrecompile gas override for HyperEVM (chainSlug 999) sequenceDiagram
participant Caller as Watcher/Relayer
participant WritePre as WritePrecompile
participant Precompile as System Precompile
Caller->>WritePre: handlePayload(queueParams)
alt chainSlug == 999 and gasLimit == 0
Note right of WritePre: overrideParams.gasLimit = 1_500_000
else gasLimit set or other chain
Note right of WritePre: use provided gasLimit or defaults
end
WritePre->>Precompile: encode and submit precompile payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas needing focused review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deployments/stage_verification.json (1)
311-340: Remove or properly register the second FeesPlug entry for network 84532.The second FeesPlug at line 333 (
0x5cdcD147A01Fc29C20c2061a89faF01F20363A20) appears only in stage_verification.json. It's not registered in stage_addresses.json, making it an orphaned entry. Across prod_addresses.json, multiple FeesPlug versions follow the naming convention: "FeesPlug" and "FeesPlugV2". Either:
- Delete the second entry if it's unused, or
- Add
"FeesPlugV2": "0x5cdcD147A01Fc29C20c2061a89faF01F20363A20"to stage_addresses.json network 84532 section and rename the verification entry accordingly.deployments/prod_addresses.json (1)
1-308: PR marked "DO NOT MERGE"—this should not be merged to production.The PR title explicitly states "DO NOT MERGE". This file contains production deployment addresses and must not be committed while the PR is in draft/experimental status. Remove this file from the prod branch, or clarify whether this is an accidental production change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/evmx/fees/Credit.sol(1 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(1 hunks)deployments/dev_verification.json(20 hunks)deployments/prod_addresses.json(21 hunks)deployments/prod_verification.json(4 hunks)deployments/stage_addresses.json(4 hunks)deployments/stage_verification.json(1 hunks)hardhat-scripts/config/config.ts(1 hunks)hardhat-scripts/utils/overrides.ts(2 hunks)
🔇 Additional comments (3)
deployments/prod_verification.json (1)
122-127: The review comment is based on incorrect analysis of the JSON structure.The Socket entry in
prod_verification.jsonis a 4-element tuple:[address, contractName, filePath, constructorParams]. The value[169, "0x7BD61c667f869FB21b77626f0Ac0ACEE51e4BE7C", "EVMX"]is the constructor parameters (4th element), not the entry itself.The full entry is:
["0xB94742B094f89A8D53e15A45CdBf9810a5B090Eb", "Socket", "contracts/protocol/Socket.sol", [169, "0x7BD61c667f869FB21b77626f0Ac0ACEE51e4BE7C", "EVMX"]]Every chain in
prod_verification.jsonuses this consistent nested structure. The code inverify.tscorrectly expects this format (typeVerifyArgs = [string, string, string, any[]]), and the parsing handles it properly viaverify(...chainParams[index]). No structural change occurred.Likely an incorrect or invalid review comment.
deployments/prod_addresses.json (2)
211-211: This review comment asks for on-chain bytecode verification, which is not a code review task.The prod_addresses.json changes are deployment configuration updates only. The contracts referenced (WritePrecompile.sol, FeesManager.sol) contain no chain-12921-specific code, no HyperEVM gas limit overrides, and no hardcoded address dependencies. Verifying what bytecode lives at 0xb8613c3BfF759Bd4596DBf4e7a57195f08454EFB or 0x22D99b38e32198D5C09C01AF872C6dBc0c875E92 on chain 12921 requires reading from the chain via RPC—this is an operational verification task, not a source code concern.
The source code changes here don't create integration risks that can be caught in code review. Address mappings in JSON are deployment artifacts separate from contract logic.
Likely an incorrect or invalid review comment.
7-7: Remove the claim about accompanying contract changes—neither Credit.sol nor WritePrecompile.sol was modified in this PR.The review incorrectly states that the AI summary references contract changes (rescue function, gas limit handling) that should accompany these address mappings. Verification shows:
contracts/evmx/fees/Credit.solcontains arescuefunction at line 381, but has zero changes in this PRcontracts/evmx/watcher/precompiles/WritePrecompile.solalready handles HyperEVM gas limits (lines 132-139), but has zero changes in this PRThe FeesPlugV2 addresses are correctly added across all networks. The identical FeesPlug/FeesPlugV2 pairs on networks 56, 100, 169, 484, 747, 5000, 57073, 59144, 80094, 747474 represent a proxy pattern—configuration that points both the old and new implementation to the same address. This is intentional and doesn't require code validation.
If you need address validation before merge, request that explicitly. Don't reference contract changes that aren't part of this PR.
Likely an incorrect or invalid review comment.
| function rescue( | ||
| uint32 chainSlug_, | ||
| address token_, | ||
| uint256 amount_, | ||
| uint256 maxFees_, | ||
| address receiver_ | ||
| ) public onlyOwner{ | ||
| // Add it to the queue and submit request | ||
| _createRequest( | ||
| chainSlug_, | ||
| address(this), | ||
| maxFees_, | ||
| abi.encodeCall(IFeesPlug.withdrawFees, (token_, receiver_, amount_)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add accounting update for tokenOnChainBalances in rescue function.
The rescue function bypasses the credit burning and tokenOnChainBalances accounting that withdrawCredits performs (lines 369-370). This creates inconsistent state if rescue is used to withdraw tokens tracked in tokenOnChainBalances.
Apply this diff to maintain consistent accounting:
function rescue(
uint32 chainSlug_,
address token_,
uint256 amount_,
uint256 maxFees_,
address receiver_
) public onlyOwner{
+ // Update accounting to match withdrawCredits behavior
+ tokenOnChainBalances[chainSlug_][token_] -= amount_;
+
// Add it to the queue and submit request
_createRequest(
chainSlug_,
address(this),
maxFees_,
abi.encodeCall(IFeesPlug.withdrawFees, (token_, receiver_, amount_))
);
}🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 381 to 395, the rescue function
currently creates a withdraw request but skips the same tokenOnChainBalances
accounting and credit burning performed in withdrawCredits (lines ~369-370);
update rescue to mirror withdrawCredits by first ensuring
tokenOnChainBalances[chainSlug_][token_] is sufficient, decrement
tokenOnChainBalances[chainSlug_][token_] by amount_, and perform the same credit
burn/accounting step used in withdrawCredits (i.e. call the same internal
credit-burning helper or logic) before creating the request so on-chain balances
and credit state stay consistent.
| } else if (queueParams_.transaction.chainSlug == 999) { | ||
| // HyperEVM default gas limit | ||
| queueParams_.overrideParams.gasLimit = 1_500_000; | ||
| } |
There was a problem hiding this comment.
Remove the redundant else-if that prevents custom gas limits for HyperEVM.
Lines 137-139 unconditionally override any non-zero gasLimit to 1,500,000 for HyperEVM (chainSlug 999). This prevents callers from setting custom gas limits. The default is already handled at lines 131-133 when gasLimit == 0.
Apply this diff to remove the redundant override:
- } else if (queueParams_.transaction.chainSlug == 999) {
- // HyperEVM default gas limit
- queueParams_.overrideParams.gasLimit = 1_500_000;
}📝 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.
| } else if (queueParams_.transaction.chainSlug == 999) { | |
| // HyperEVM default gas limit | |
| queueParams_.overrideParams.gasLimit = 1_500_000; | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/watcher/precompiles/WritePrecompile.sol around lines 137 to
140, remove the redundant else-if block that unconditionally sets
queueParams_.overrideParams.gasLimit = 1_500_000 for chainSlug == 999 so callers
can supply custom non-zero gas limits; keep the existing default-setting logic
at lines 131-133 which should only set the HyperEVM default when gasLimit == 0,
and ensure no other code path overwrites a provided non-zero gasLimit for
chainSlug 999.
| [ChainSlug.POLYGON_MAINNET]: { | ||
| gasPriceMultiplier: 2, | ||
| gasLimit: 3_000_000, | ||
| // gasPriceMultiplier: 2, | ||
| gasLimit: 3_00_000, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the 10x gasLimit reduction for POLYGON_MAINNET.
The gasLimit changed from 3,000,000 to 300,000 (10x reduction). This could cause transaction failures if deployments or operations on Polygon require more than 300K gas.
🏁 Script executed:
#!/bin/bash
# Verify the impact of gasLimit changes on POLYGON_MAINNET transactions
rg -nC5 'POLYGON_MAINNET' --type=ts | head -50Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Get the full context of the overrides.ts file around the changed lines
cat hardhat-scripts/utils/overrides.ts | head -60Length of output: 1419
🏁 Script executed:
#!/bin/bash
# Find what imports or uses the overrides from overrides.ts
rg -nC3 "overrides\[.*POLYGON" --type=tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for usage of gasLimit in deployment or transaction contexts
rg -nC3 "gasLimit" --type=ts | grep -A3 -B3 "POLYGON\|deploy"Length of output: 1034
🏁 Script executed:
#!/bin/bash
# Get the full context of the overrides.ts file around the changed lines
cat hardhat-scripts/utils/overrides.ts | head -60Length of output: 1419
🏁 Script executed:
#!/bin/bash
# Find what imports or uses the overrides from overrides.ts
rg -nC3 "overrides\[" --type=ts | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for usage of gasLimit in deployment or transaction contexts
rg -nP "gasLimit" --type=ts | head -30Length of output: 1513
🏁 Script executed:
#!/bin/bash
# Check git diff to see what changed in this PR for the overrides file
git diff HEAD~1 hardhat-scripts/utils/overrides.ts 2>/dev/null || echo "Could not get git diff"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify this is actually a typo by checking number formatting consistency
rg -n "gasLimit: [0-9_]+" hardhat-scripts/utils/overrides.tsLength of output: 496
Fix typo in POLYGON_MAINNET gasLimit: wrong underscore placement causes 10x reduction.
Line 45 has gasLimit: 3_00_000 which evaluates to 300,000. This is a formatting error—the underscore is in the wrong position. Every other gasLimit in the file follows X_000_000 format (50_000_000, 3_000_000, 2_000_000, etc.). The correct value should be gasLimit: 3_000_000, to match the pattern used for BASE_SEPOLIA (line 29), HARMONY (line 66), and GNOSIS (line 79). The current 300K limit is far too low for Polygon deployments and will cause transaction failures.
[ChainSlug.POLYGON_MAINNET]: {
// gasPriceMultiplier: 2,
gasLimit: 3_000_000,
🤖 Prompt for AI Agents
In hardhat-scripts/utils/overrides.ts around lines 43 to 45, the POLYGON_MAINNET
gasLimit has a misplaced underscore producing 300,000 instead of the intended
3,000,000; update the gasLimit value to use the correct numeric literal format
(3_000_000) to match other entries and ensure sufficient gas for Polygon
deployments.
| ); | ||
| } | ||
|
|
||
| function rescue( |
| } else { | ||
| queueParams_.overrideParams.gasLimit = 10_000_000; // other chains default gas limit | ||
| } | ||
| } else if (queueParams_.transaction.chainSlug == 999) { |
Summary by CodeRabbit
New Features
Chores
Configuration