Gas account #218
Gas account #218
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (9)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR migrates the protocol's fee management from a credits-based system (FeesManager, FeesPlug, Credit) to a gas-based system (GasAccountManager, GasStation, GasAccountToken, GasEscrow), including corresponding renames in core infrastructure (SocketFeeManager → NetworkFeeCollector, FeesPool → GasVault), contract refactors, and deployment/test suite updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/App
participant Token as GasAccountToken
participant GAM as GasAccountManager
participant Escrow as GasEscrow
participant Watcher as Watcher
participant GasStation as GasStation
User->>Token: wrapToGas(user) {send ETH}
Token->>Token: mint(user, gasAmount)
User->>Token: approve(GasAccountManager, amount)
User->>GAM: withdrawToChain(chainSlug, token, amount, maxFees, receiver)
GAM->>Token: burn(user, amount)
GAM->>Watcher: _submitPayload(...)
GAM->>Escrow: escrowGas(payloadId, user, amount)
Escrow->>Escrow: accountEscrow[user] += amount<br/>payloadEscrow[payloadId] = Active
rect rgb(220, 240, 255)
Note over Watcher,Escrow: Payload Execution & Settlement
Watcher->>Watcher: executePayload(payloadId)
Watcher->>Escrow: releaseEscrow(payloadId)
Escrow->>Escrow: accountEscrow[user] -= amount<br/>payloadEscrow[payloadId] = Released
Watcher->>Escrow: settleGasPayment(payloadId, transmitter, fee)
Escrow->>Escrow: accountEscrow[user] -= fee<br/>payloadEscrow[payloadId] = Settled
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
contracts/evmx/watcher/precompiles/WritePrecompile.sol (2)
160-200: This should initialize gasLimit from the override or use defaults—currently loses user-provided values.The
_validatefunction has a logic error: whenrawPayload_.overrideParams.gasLimit != 0, the function never assigns it to the return variable and returns 0 instead. The intended behavior appears to be "use provided gasLimit if non-zero, otherwise apply chain-specific defaults."Apply this diff:
function _validate( RawPayload calldata rawPayload_, address appGateway_ ) internal view returns (uint256 gasLimit) { if ( rawPayload_.overrideParams.value > chainMaxMsgValueLimit[rawPayload_.transaction.chainSlug] ) revert MaxMsgValueLimitExceeded(); if ( rawPayload_.transaction.payload.length == 0 || rawPayload_.transaction.payload.length > PAYLOAD_SIZE_LIMIT ) { revert InvalidPayloadSize(); } if (rawPayload_.transaction.target == bytes32(0)) revert InvalidTarget(); watcher__.verifyConnections( rawPayload_.transaction.chainSlug, rawPayload_.transaction.target, appGateway_, rawPayload_.overrideParams.switchboardType ); // todo: can be changed to set the default gas limit for each chain - if (rawPayload_.overrideParams.gasLimit == 0) { + gasLimit = rawPayload_.overrideParams.gasLimit; + if (gasLimit == 0) { if (rawPayload_.transaction.chainSlug == 5000) { // Mantle default gas limit gasLimit = 8_000_000_000; } else if (rawPayload_.transaction.chainSlug == 1329) { // Sei default gas limit gasLimit = 8_000_000; } else if (rawPayload_.transaction.chainSlug == 999) { // HyperEVM default gas limit gasLimit = 1_500_000; } else { gasLimit = 10_000_000; // other chains default gas limit } } }
158-158: Fix NatSpec formatting: use///instead of//for comment lines.Line 158 uses
// @paramwhich breaks the NatSpec documentation format. This line won't be parsed by documentation generators and IDE hover-hints won't work.- // @param rawPayload_ The queue parameters to process + /// @param rawPayload_ The queue parameters to processscript/counter/WithdrawFeesArbitrumFeesPlug.s.sol (1)
13-13: Update stale comment to match new API.The comment references
withdrawFeeTokens(), but the script callswithdrawToChain()at line 52. Update the comment to reflect the current implementation.-// @dev Make sure your app has withdrawFeeTokens() function implemented. You can check its implementation in CounterAppGateway.sol +// @dev Make sure your app has withdrawToChain() function implemented. You can check its implementation in CounterAppGateway.soltest/SetupTest.t.sol (1)
421-449: HonorgasAmount_instead of hard-coding 100 etherThis should mint, approve, deposit, and assert against the caller-supplied
gasAmount_. Keeping100 etherhard-coded means the helper silently does the wrong thing as soon as a test wants to exercise any other amount (minted balance, approval, station balance delta, and emitted event no longer line up). Replace the literals with the parameter so the helper reflects its signature.- token.mint(address(user_), 100 ether); + token.mint(address(user_), gasAmount_); @@ - token.approve(address(socketConfig.gasStation), 100 ether); - socketConfig.gasStation.depositForGasAndNative(address(token), user_, 100 ether); + token.approve(address(socketConfig.gasStation), gasAmount_); + socketConfig.gasStation.depositForGasAndNative(address(token), user_, gasAmount_); @@ - gasStationBalance + 100 ether, + gasStationBalance + gasAmount_,hardhat-scripts/test/chainTest.ts (1)
76-112: Fix outdated GasAccountManager callsThis should stop calling
getPayloadEscrowandbalanceOf; IGasAccountManager only exposesavailableGas,totalGas, andescrowedGas, so the current ABI/calls will revert once the new contract is deployed. Update the ABI entries and the balance reads to the new getters so the script can still print available vs escrowed gas.- const gasAccountManagerABI = [ - "function totalGas(address) view returns (uint256)", - "function getPayloadEscrow(address) view returns (uint256)", - "function balanceOf(address) view returns (uint256)", - ]; + const gasAccountManagerABI = [ + "function totalGas(address) view returns (uint256)", + "function availableGas(address) view returns (uint256)", + "function escrowedGas(address) view returns (uint256)", + ]; ... - const payloadEscrow = await this.gasAccountManager.getPayloadEscrow( - appGatewayAddress - ); - const availableFees = await this.gasAccountManager.balanceOf( - appGatewayAddress - ); + const availableGas = + await this.gasAccountManager.availableGas(appGatewayAddress); + const payloadEscrow = + await this.gasAccountManager.escrowedGas(appGatewayAddress);contracts/evmx/plugs/GasStation.sol (2)
44-51: Potential precision loss in deposit splitting.The integer division at line 49 causes precision loss for small deposits. For
amount_values 1-9,nativeAmount_becomes 0, resulting in the full amount going to gas instead of the intended 90/10 split.Consider adding a minimum deposit check or documenting this behavior:
function depositForGasAndNative( address token_, address receiver_, uint256 amount_ ) external override { + if (amount_ < 10) revert InvalidDepositAmount(); uint256 nativeAmount_ = amount_ / 10; _deposit(token_, receiver_, amount_ - nativeAmount_, nativeAmount_); }
89-106: Incorrect interface casting for token operations.Lines 94-95 cast
token_toIGasAccountToken, buttoken_represents the actual ERC20 token being withdrawn (e.g., USDC), not the GasAccountToken (SGAS). This will cause runtime failures when the watcher calls this function.Apply this diff to use the correct interface:
function withdrawToTokens( address token_, address receiver_, uint256 amount_ ) external override onlySocket { - uint256 balance = IGasAccountToken(token_).balanceOf(address(this)); - uint8 decimals = IGasAccountToken(token_).decimals(); + uint256 balance = IERC20(token_).balanceOf(address(this)); + uint8 decimals = IERC20Metadata(token_).decimals(); if (decimals < 18) { amount_ = amount_ / 10 ** (18 - decimals); } else if (decimals > 18) { amount_ = amount_ * 10 ** (decimals - 18); } if (balance < amount_) revert InsufficientTokenBalance(token_, balance, amount_); token_.safeTransfer(receiver_, amount_); emit GasWithdrawn(token_, receiver_, amount_); }Note: You'll need to add the appropriate import:
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";Or use a compatible interface that provides
balanceOfanddecimals.
🧹 Nitpick comments (11)
script/counter/WithdrawFeesArbitrumFeesPlug.s.sol (1)
6-6: Remove unused import.
GasAccountManageris imported but never used in this script. Remove this import to keep the dependencies clean.-import {GasAccountManager} from "../../contracts/evmx/fees/GasAccountManager.sol";hardhat-scripts/deploy/6.connect.ts (1)
22-23: Clean up or clarify the commented line.The commented line still references the old
FeesPlugwhich has been replaced byGasStation. Either remove this comment entirely or update it to reflect the new naming convention.Apply this diff to update the comment:
-// const plugs = [Contracts.ContractFactoryPlug, Contracts.GasStation]; +// const plugs = [Contracts.ContractFactoryPlug, Contracts.GasStation]; const plugs = [Contracts.GasStation];Or remove it if
ContractFactoryPlugis no longer relevant.hardhat-scripts/deploy/8.setupEnv.ts (2)
35-35: Consider renaming environment variable for clarity.The environment variable
FEES_MANAGERnow points toGasAccountManager. Consider renaming the env var toGAS_ACCOUNT_MANAGERto match the new contract name and avoid confusion.
43-45: Consider renaming environment variable for clarity.Similar to
FEES_MANAGER, theARBITRUM_FEES_PLUGenvironment variable now points toGasStation. Consider renaming it toARBITRUM_GAS_STATIONfor consistency with the new naming convention.hardhat-scripts/deploy/5.fundTransfers.ts (1)
6-6: Update constant name to match new terminology.The constant
FEES_POOL_FUNDING_AMOUNT_THRESHOLDstill references the old "FeesPool" terminology. It should be renamed toGAS_VAULT_FUNDING_AMOUNT_THRESHOLDin the config file for consistency with the migration.Also applies to: 21-21, 32-32
hardhat-scripts/deploy/9.setupTransmitter.ts (1)
42-44: Use a clearer variable name for gas balance.The variable name
gasis ambiguous. It should begasBalanceto clearly indicate it holds a balance value, not the gas itself.Apply this diff:
- const gas = await gasAccountManagerContract + const gasBalance = await gasAccountManagerContract .connect(transmitterSigner) .balanceOf(transmitter);- if (gas.lt(TRANSMITTER_GAS_THRESHOLD)) { + if (gasBalance.lt(TRANSMITTER_GAS_THRESHOLD)) {PAYLOAD_ID_ARCHITECTURE.md (1)
11-21: Add a language hint to the fenced block.
This should specify a language (e.g.text) for the bit‑layout fence so markdownlint (MD040) stays green; the rest of the doc already passes lint.contracts/evmx/fees/GasAccountToken.sol (1)
63-66: Consider defensive check for escrow accounting.If escrow accounting has a bug causing
escrowed > total, this subtraction will revert. While GasEscrow should prevent this, a defensive check or clear revert message would improve debugging.function balanceOf(address account) public view override returns (uint256) { - return - super.balanceOf(account) - addressResolver__.gasEscrow__().getEscrowedAmount(account); + uint256 total = super.balanceOf(account); + uint256 escrowed = addressResolver__.gasEscrow__().getEscrowedAmount(account); + if (escrowed > total) revert InvalidEscrowState(); + return total - escrowed; }Alternatively, document that GasEscrow invariants guarantee
escrowed <= total.contracts/evmx/fees/GasAccountManager.sol (3)
69-80: Reorder operations to transfer ETH before minting tokens.Minting tokens before transferring ETH to the vault temporarily breaks the invariant that total SGAS supply should be backed by vault holdings. While the revert behavior is safe, the order violates best practices.
function wrapToGas(address receiver) external payable override onlyWatcher { uint256 amount = msg.value; if (amount == 0) revert InvalidAmount(); + // Transfer ETH to vault first to maintain backing invariant + SafeTransferLib.safeTransferETH(address(gasVault__()), amount); + // Mint tokens to receiver gasAccountToken__().mint(receiver, amount); - // reverts if transfer fails - SafeTransferLib.safeTransferETH(address(gasVault__()), amount); emit GasWrapped(receiver, amount); }
113-123: Document or emit event for native-to-gas conversion fallback.When vault withdrawal fails (lines 115-122), the user receives SGAS instead of native tokens without explicit notification. While the accounting is correct, this changes what the user receives from their deposit.
Consider adding a separate event:
emit NativeConvertedToGas(chainSlug, depositTo, nativeAmount);Or update the
Depositedevent to clearly indicate final amounts:-emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount); +// gasAmount and nativeAmount now reflect what user actually received +emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount);The current implementation already updates
gasAmountandnativeAmountfor the event (lines 120-121), which is good. Verify this behavior is clearly documented in the event definition.
247-257: Use custom error for consistency.Line 251 uses a string literal error instead of a custom error, inconsistent with the rest of the codebase.
function setChainMaxFees( uint32[] calldata chainSlugs_, uint256[] calldata maxFees_ ) external onlyOwner { - if (chainSlugs_.length != maxFees_.length) revert("Array length mismatch"); + if (chainSlugs_.length != maxFees_.length) revert ArrayLengthMismatch(); for (uint256 i = 0; i < chainSlugs_.length; i++) { maxGasPerChainSlug[chainSlugs_[i]] = maxFees_[i]; emit MaxGasPerChainSlugSet(chainSlugs_[i], maxFees_[i]); } }Add error definition:
error ArrayLengthMismatch();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
PAYLOAD_ID_ARCHITECTURE.md(7 hunks)contracts/evmx/base/AppGatewayBase.sol(3 hunks)contracts/evmx/fees/Credit.sol(0 hunks)contracts/evmx/fees/FeesManager.sol(0 hunks)contracts/evmx/fees/GasAccountManager.sol(1 hunks)contracts/evmx/fees/GasAccountToken.sol(1 hunks)contracts/evmx/fees/GasEscrow.sol(1 hunks)contracts/evmx/fees/GasVault.sol(3 hunks)contracts/evmx/fees/MessageResolver.sol(4 hunks)contracts/evmx/helpers/AddressResolver.sol(3 hunks)contracts/evmx/helpers/AddressResolverUtil.sol(2 hunks)contracts/evmx/helpers/AsyncDeployer.sol(1 hunks)contracts/evmx/helpers/AsyncPromise.sol(2 hunks)contracts/evmx/helpers/solana-utils/program-pda/GasStationPdas.sol(1 hunks)contracts/evmx/interfaces/IAddressResolver.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(0 hunks)contracts/evmx/interfaces/IGasAccountManager.sol(1 hunks)contracts/evmx/interfaces/IGasAccountToken.sol(3 hunks)contracts/evmx/interfaces/IGasEscrow.sol(1 hunks)contracts/evmx/interfaces/IGasStation.sol(1 hunks)contracts/evmx/interfaces/IGasVault.sol(1 hunks)contracts/evmx/plugs/GasStation.sol(5 hunks)contracts/evmx/watcher/Watcher.sol(12 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(2 hunks)contracts/protocol/NetworkFeeCollector.sol(2 hunks)contracts/protocol/Socket.sol(4 hunks)contracts/protocol/SocketConfig.sol(6 hunks)contracts/protocol/interfaces/INetworkFeeCollector.sol(1 hunks)contracts/protocol/interfaces/ISocket.sol(1 hunks)contracts/protocol/switchboard/FastSwitchboard.sol(3 hunks)contracts/protocol/switchboard/MessageSwitchboard.sol(8 hunks)contracts/utils/Pausable.sol(0 hunks)contracts/utils/common/AccessRoles.sol(1 hunks)contracts/utils/common/Converters.sol(1 hunks)contracts/utils/common/Errors.sol(1 hunks)contracts/utils/common/IdUtils.sol(2 hunks)contracts/utils/common/Structs.sol(1 hunks)hardhat-scripts/admin/disconnect.ts(6 hunks)hardhat-scripts/admin/rescue.ts(1 hunks)hardhat-scripts/config/config.ts(7 hunks)hardhat-scripts/constants/constants.ts(1 hunks)hardhat-scripts/deploy/1.deploy.ts(3 hunks)hardhat-scripts/deploy/2.roles.ts(3 hunks)hardhat-scripts/deploy/3.configureChains.ts(4 hunks)hardhat-scripts/deploy/4.configureEVMx.ts(3 hunks)hardhat-scripts/deploy/5.fundTransfers.ts(2 hunks)hardhat-scripts/deploy/6.connect.ts(1 hunks)hardhat-scripts/deploy/8.setupEnv.ts(1 hunks)hardhat-scripts/deploy/9.setupTransmitter.ts(3 hunks)hardhat-scripts/test/chainTest.ts(6 hunks)hardhat-scripts/utils/gatewayId.ts(1 hunks)script/counter/WithdrawFeesArbitrumFeesPlug.s.sol(3 hunks)script/helpers/CheckDepositedCredits.s.sol(0 hunks)script/helpers/CheckDepositedGas.s.sol(1 hunks)script/helpers/DepositGas.s.sol(1 hunks)script/helpers/DepositGasAndNative.s.sol(1 hunks)script/helpers/DepositGasMainnet.s.sol(2 hunks)script/helpers/TransferRemainingGas.s.sol(1 hunks)script/helpers/WithdrawRemainingCredits.s.sol(0 hunks)script/helpers/WithdrawRemainingGas.s.sol(1 hunks)src/enums.ts(3 hunks)src/events.ts(1 hunks)src/types.ts(1 hunks)test/PausableTest.t.sol(4 hunks)test/SetupTest.t.sol(17 hunks)test/SocketPayloadIdVerification.t.sol(16 hunks)test/apps/Counter.t.sol(1 hunks)test/apps/counter/CounterAppGateway.sol(3 hunks)test/mocks/MockPlug.sol(2 hunks)test/switchboard/MessageSwitchboard.t.sol(30 hunks)
💤 Files with no reviewable changes (6)
- contracts/evmx/fees/FeesManager.sol
- script/helpers/CheckDepositedCredits.s.sol
- contracts/utils/Pausable.sol
- script/helpers/WithdrawRemainingCredits.s.sol
- contracts/evmx/interfaces/IFeesManager.sol
- contracts/evmx/fees/Credit.sol
🧰 Additional context used
🪛 LanguageTool
PAYLOAD_ID_ARCHITECTURE.md
[grammar] ~136-~136: Ensure spelling is correct
Context: ...tches local config 2. Source (Watcher offchain): Verifies origin component matches e...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
PAYLOAD_ID_ARCHITECTURE.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (52)
contracts/protocol/switchboard/FastSwitchboard.sol (1)
1-186: Formatting-only changes; behavior unchanged.The file contains purely cosmetic modifications: blank line additions, docstring reformatting, and argument comment reorganization. The core logic and function signatures remain identical. No functional issues detected.
test/PausableTest.t.sol (1)
1-278: Formatting improvements look good.The changes split long function calls across multiple lines and add blank lines for better readability. No logic or functional changes introduced.
contracts/evmx/helpers/solana-utils/program-pda/GasStationPdas.sol (1)
6-6: LGTM - Library rename fully propagated.The library rename from
FeesPlugProgramPdatoGasStationProgramPdais complete. Verification found zero remaining references to the old library name across the entire codebase. All imports and usages have been updated. No code quality issues.script/counter/WithdrawFeesArbitrumFeesPlug.s.sol (3)
18-19: Resolver-based setup looks correct.The migration to using
AddressResolverto obtain system components is the right approach. The double cast at line 19 may be redundant depending on whatgasAccountToken__()returns, but this is consistent with resolver patterns.
24-24: Balance check correctly migrated.The change from
feesManager.balanceOftogasAccountToken.balanceOfcorrectly reflects the migration to the gas account system.
52-52: Withdrawal call correctly updated.The migration from
withdrawCreditstowithdrawToChainis correct. The parameters align with the new gas account system API, and the chain ID (421614) is valid for Arbitrum Sepolia.contracts/utils/common/Errors.sol (1)
1-73: Removed errors are clean—only archived code references them.Verification confirms removed errors have no references in active code. The only matches are in the
deprecated/directory (RequestAlreadySettled, OnlyRequestHandlerAllowed, OnlyPromiseResolverAllowed, RequestAlreadyCancelled), which contains legacy implementations. The migration is sound and won't break active functionality.script/helpers/TransferRemainingGas.s.sol (4)
6-6: LGTM on imports and contract rename.The imports align with the gas-based system migration, and the contract name is appropriately updated.
Also applies to: 8-9, 11-11
18-24: LGTM on AddressResolver integration.The initialization pattern using AddressResolver to retrieve contract instances is appropriate.
36-45: LGTM on token transfer logic.The available fees calculation is correct, and the low-level call pattern with error handling is acceptable for script usage.
28-29: No action required. The arithmetic is correct.The relationship holds:
totalGas >= gasAccountToken.balanceOf(appGateway)is guaranteed by design, not a risky assumption.balanceOf()explicitly subtracts escrow from the samesuper.balanceOf()that backstotalBalanceOf(), so the subtraction in lines 28-29 will never underflow. The calculation correctly derives the escrowed amount.contracts/protocol/interfaces/ISocket.sol (1)
62-62: LGTM - Documentation terminology improved.The comment update from "request" to "payload" better aligns with the payload-centric architecture used throughout the codebase.
contracts/utils/common/IdUtils.sol (1)
37-62: LGTM - Formatting improvements.The multi-line function signatures improve readability without affecting functionality.
contracts/protocol/switchboard/MessageSwitchboard.sol (1)
118-355: LGTM - Formatting and documentation improvements.The changes improve code readability through better formatting and inline comments. No functional impact.
hardhat-scripts/constants/constants.ts (1)
17-17: LGTM - Whitespace normalization.contracts/evmx/helpers/AsyncDeployer.sol (1)
56-60: LGTM - Formatting improvement.Multi-line function signature improves readability without affecting functionality.
test/switchboard/MessageSwitchboard.t.sol (1)
88-1153: LGTM - Test formatting improvements.All changes are formatting refinements (whitespace, line breaks) with no impact on test logic or coverage.
contracts/utils/common/Structs.sol (1)
32-44: LGTM - Well-designed escrow state management.The new
EscrowStateenum andEscrowEntrystruct provide a clean foundation for the gas escrow system. The state values (None, Active, Released, Settled) cover the complete escrow lifecycle, and the struct captures all necessary metadata (account, amount, timestamp, state).hardhat-scripts/admin/rescue.ts (1)
73-75: LGTM - Contract address migration aligned with gas-based architecture.The script now correctly references
SocketGasAccountManagerandGasStationinstead of the deprecatedSocketFeesManagerandFeesPlug, consistent with the PR's migration to a gas-based fee management system.contracts/evmx/interfaces/IGasVault.sol (1)
4-11: LGTM - Clean interface migration.The rename from
IFeesPooltoIGasVaultandgetBalance()tovaultBalance()is straightforward and aligns with the gas-based architecture migration. The function signatures preserve the same semantics.test/apps/Counter.t.sol (1)
19-19: LGTM - Test updated for gas-based flow.The rename from
depositNativeAndCreditstodepositNativeAndGascorrectly aligns the test with the new gas-based accounting system.test/SocketPayloadIdVerification.t.sol (1)
1-403: LGTM - Formatting changes only.This file contains only formatting and whitespace adjustments with no functional changes to payload ID verification logic.
hardhat-scripts/utils/gatewayId.ts (1)
16-19: LGTM - Gateway ID mapping updated.The update from
FeesPlug/FeesManagertoGasStation/GasAccountManageris consistent with the broader architecture migration. The logic and error handling remain intact.src/events.ts (1)
10-10: LGTM - Events migrated to gas terminology.The rename from
feesPlugEventstogasStationEventsandFeesDepositedtoGasDepositedcorrectly reflects the new gas-based architecture.contracts/protocol/Socket.sol (3)
83-88: LGTM - Payload ID verification formatting improved.The reformatted
getVerificationInfocall improves readability without changing logic. The verification checks for chain slug and switchboard ID remain correct.
248-256: LGTM - Formatting improvement.The reformatted function signature for
_verifyPlugSwitchboardimproves readability without changing the logic.
167-172: No issues found. The interface migration is correct and complete.All three concerns are verified:
Interface signature matches —
INetworkFeeCollector.collectNetworkFee(ExecuteParams, TransmissionParams)is properly defined as external payable and matches the implementation in NetworkFeeCollector.Fee handling preserved — NetworkFeeCollector validates incoming fees with
if (msg.value < networkFee) revert InsufficientFees()and emits NetworkFeeCollected event.SocketConfig properly initialized — networkFeeCollector is declared as
INetworkFeeCollector public networkFeeCollectorwith a setter functionsetNetworkFeeCollector()protected by GOVERNANCE_ROLE and an accompanying event emission.Socket.sol correctly calls the method with value via
{value: transmissionParams_.socketFees}and passes the right parameters. The oldpayAndCheckFeescode path exists only in deprecated test files and is not used in production.contracts/utils/common/AccessRoles.sol (1)
15-16: No issues found. Role migration is complete and consistent.Verification confirms all active code properly uses
GAS_MANAGER_ROLE. The role is correctly defined, granted in the constructor, and enforced viaonlyRolemodifier inGasVault.sol. Active test setup references the new role. The singleFEE_MANAGER_ROLEreference exists only in a deprecated test file, which poses no functional concern.script/helpers/DepositGasAndNative.s.sol (2)
6-6: LGTM!The contract and type references have been updated consistently from
FeesPlugtoGasStation, aligning with the broader migration to gas-based terminology.Also applies to: 10-10, 17-17
24-24: LGTM!The approval and deposit calls have been correctly updated to use the
gasStationvariable and the renameddepositForGasAndNativemethod.Also applies to: 33-33
hardhat-scripts/deploy/2.roles.ts (1)
28-28: LGTM!The role assignments have been correctly migrated from
FeesPool/FeesPlugtoGasVault/GasStation, withGasAccountManagerproperly receiving theFEE_MANAGER_ROLEon theGasVaultcontract.Also applies to: 39-39, 122-128
hardhat-scripts/deploy/5.fundTransfers.ts (1)
12-39: LGTM!The function and variable names have been consistently updated from
FeesPooltoGasVaultterminology. The funding logic remains intact.test/mocks/MockPlug.sol (1)
37-40: LGTM!The new
triggerSocketmethod correctly forwards both data and value to the socket'ssendPayloadfunction, providing useful test functionality.contracts/evmx/fees/MessageResolver.sol (3)
76-76: LGTM!Comments and error definitions have been correctly updated to reflect the migration from credit-based to gas-based accounting.
Also applies to: 107-108, 239-239
268-272: LGTM!The sponsor gas availability check has been correctly updated to use
gasAccountToken__().isGasAvailable(), aligning with the new gas-based accounting system.
278-283: LGTM!The gas transfer from sponsor to transmitter has been correctly updated to use
gasAccountToken__().transferFrom(), maintaining the same parameters and error handling.test/apps/counter/CounterAppGateway.sol (2)
19-19: LGTM!The state variable has been correctly renamed from
feesManagerSwitchtogasAccountManagerSwitchto align with the new gas-based terminology.
98-105: LGTM!The function has been correctly renamed from
withdrawCreditstowithdrawToChain, with the internal call and callback selector updated accordingly. This aligns with the gas-based terminology migration.Also applies to: 118-118
contracts/evmx/base/AppGatewayBase.sol (3)
8-8: LGTM on import change from IERC20 to IGasAccountToken.This aligns with the migration to gas-based token interactions.
170-184: No issues found—withdrawToChain signature matches IGasAccountManager interface.The function call in
_withdrawToChainpasses the correct parameters in the correct order:chainSlug_(uint32),token_(address),amount_(uint256),overrideParams.maxFees(uint256), andreceiver_(address). This matches the interface signature exactly.
162-162: IGasAccountToken interface correctly declares permit method with matching signature.Verification confirms the interface at
contracts/evmx/interfaces/IGasAccountToken.soldeclarespermit(address spender, uint256 value, uint256 deadline, uint256 nonce, bytes memory signature)matching the call at line 162. No issues found.script/helpers/DepositGasMainnet.s.sol (1)
11-36: LGTM on migration from FeesPlug to GasStation.The contract rename, variable updates, and method call changes are consistent with the gas-based architecture migration.
hardhat-scripts/deploy/1.deploy.ts (3)
117-127: LGTM on FeesPool to GasVault migration.The deployment logic correctly checks for an existing feePool and either deploys a new GasVault or maps the existing address. The contract path and parameters are correct.
143-155: LGTM on FeesManager to GasAccountManager migration.The deployment correctly uses the new GasAccountManager contract with GasVault reference, and all initialization parameters are properly updated.
302-310: LGTM on FeesPlug to GasStation migration.The Socket deployment correctly updates to GasStation with appropriate contract path and parameters.
hardhat-scripts/config/config.ts (1)
17-17: LGTM on configuration migration to gas-based naming.The changes consistently rename feesPlugChains to gasStationChains, update the accessor function, and rename TRANSMITTER_CREDIT_THRESHOLD to TRANSMITTER_GAS_THRESHOLD. The fallback logic for gasStationChains is preserved correctly.
Also applies to: 168-173, 295-295
hardhat-scripts/deploy/9.setupTransmitter.ts (1)
48-53: wrapToGas method exists with correct signature—no issues found.Verification confirms the method is defined in IGasAccountManager with the expected signature:
function wrapToGas(address receiver) external payable;. The call correctly passes the transmitter address as the receiver parameter and the transaction value through the options object, which is standard ethers.js pattern.script/helpers/CheckDepositedGas.s.sol (1)
16-17: No action needed. The IGasAccountToken interface correctly declares totalBalanceOf.The verification confirms the method is properly declared in the interface at
contracts/evmx/interfaces/IGasAccountToken.solwith signaturefunction totalBalanceOf(address account) external view returns (uint256);and is correctly implemented inGasAccountToken.sol. The script call is valid.contracts/evmx/fees/GasAccountToken.sol (2)
73-78: LGTM - Transfer correctly checks available balance.The override properly ensures users can only transfer non-escrowed gas by using the escrow-aware
balanceOf.
88-91: Review the watcher approval pattern flagged by TODO.The TODO comment at line 88 and the automatic approval for watcher at lines 89-90 suggest this logic needs review. Setting approval to the exact transfer amount on each call is inefficient and unusual for ERC20 patterns.
Questions to verify:
- Why does watcher need automatic approval rather than a one-time high allowance?
- Is there a security reason for setting approval to exact
amount_each time?- Should this approval be set before or after the
isGasAvailablecheck?If the watcher pattern is intentional, document the reasoning and remove the TODO. Otherwise, consider using a standard allowance pattern.
contracts/evmx/fees/GasAccountManager.sol (2)
136-152: Verify escrow timing for bridgeFee.The function checks that user has
amount + bridgeFee(line 140) but only burnsamount(line 143). ThebridgeFeeis intended to be escrowed later when the watcher processes the payload. This creates a time window where the user could spend the remaining balance, causing the escrow to fail.Verify this is the intended flow:
- withdrawToChain checks and burns only
amount- User must maintain
bridgeFeebalance until watcher escrows it- If user spends the remaining balance before escrow, payload submission fails
If this race condition is acceptable, document it. Otherwise, consider escrowing the bridgeFee immediately:
function withdrawToChain( uint32 chainSlug, address token, uint256 amount, uint256 bridgeFee, address receiver ) external { address consumeFrom = msg.sender; // Check if amount is available in fees plug uint256 gasBalance = gasAccountToken__().balanceOf(consumeFrom); if (gasBalance < amount + bridgeFee) revert InsufficientGasAvailable(); - // Burn tokens from sender - gasAccountToken__().burn(consumeFrom, amount); + // Burn amount and escrow bridgeFee + gasAccountToken__().burn(consumeFrom, amount + bridgeFee); tokenOnChainBalances[chainSlug][toBytes32Format(token)] -= amount;But verify this matches the intended escrow accounting pattern used elsewhere.
195-204: LGTM - Escrow settlement logic is correct.The settlement flow properly releases escrowed gas, burns from the consumer, and mints to the transmitter. The two-step process (release then burn) ensures escrow accounting is updated before balance changes.
| function initialize( | ||
| address addressResolver_, | ||
| address owner_, | ||
| uint256 fees_, | ||
| bytes32 sbType_, | ||
| address forwarderSolana_ | ||
| ) external reinitializer(1) { | ||
| forwarderSolana = ForwarderSolana(forwarderSolana_); | ||
| overrideParams = overrideParams.setSwitchboardType(sbType_).setMaxFees(fees_); | ||
| _initializeOwner(owner_); | ||
| _initializeAppGateway(addressResolver_); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add parameter validation in initialize function.
The initialize function should validate critical parameters to prevent misconfiguration.
function initialize(
address addressResolver_,
address owner_,
uint256 fees_,
bytes32 sbType_,
address forwarderSolana_
) external reinitializer(1) {
+ if (addressResolver_ == address(0)) revert InvalidReceiver();
+ if (owner_ == address(0)) revert InvalidReceiver();
+ if (forwarderSolana_ == address(0)) revert InvalidReceiver();
forwarderSolana = ForwarderSolana(forwarderSolana_);
overrideParams = overrideParams.setSwitchboardType(sbType_).setMaxFees(fees_);
_initializeOwner(owner_);
_initializeAppGateway(addressResolver_);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/evmx/fees/GasAccountManager.sol around lines 54 to 65, the
initialize function lacks validation of incoming parameters; add require checks
at the top of the function to revert on invalid inputs: require(addressResolver_
!= address(0), "Invalid addressResolver"); require(owner_ != address(0),
"Invalid owner"); require(forwarderSolana_ != address(0), "Invalid
forwarderSolana"); require(fees_ > 0, "Invalid fees"); require(sbType_ !=
bytes32(0), "Invalid sbType"); perform these validations before assigning
forwarderSolana, setting overrideParams, or calling
_initializeOwner/_initializeAppGateway so the function fails fast on
misconfiguration.
| /// @notice Unwrap SGAS to native tokens | ||
| function unwrapFromGas(uint256 amount, address receiver) external onlyWatcher { | ||
| if (gasAccountToken__().balanceOf(msg.sender) < amount) revert InsufficientGasAvailable(); | ||
|
|
||
| // Burn tokens from sender | ||
| gasAccountToken__().burn(msg.sender, amount); | ||
|
|
||
| bool success = gasVault__().withdraw(receiver, amount); | ||
| if (!success) revert InsufficientBalance(); | ||
|
|
||
| emit GasUnwrapped(msg.sender, amount); | ||
| } |
There was a problem hiding this comment.
Critical: unwrapFromGas burns from wrong account.
The function burns from msg.sender (the watcher) instead of from the user who wants to unwrap. This breaks the intended flow where users unwrap their own SGAS to receive native tokens.
This function should either:
Option 1: Burn from the receiver's account (parallel to wrapToGas):
-function unwrapFromGas(uint256 amount, address receiver) external onlyWatcher {
- if (gasAccountToken__().balanceOf(msg.sender) < amount) revert InsufficientGasAvailable();
+function unwrapFromGas(address from, uint256 amount, address receiver) external onlyWatcher {
+ if (gasAccountToken__().balanceOf(from) < amount) revert InsufficientGasAvailable();
// Burn tokens from sender
- gasAccountToken__().burn(msg.sender, amount);
+ gasAccountToken__().burn(from, amount);
bool success = gasVault__().withdraw(receiver, amount);
if (!success) revert InsufficientBalance();
- emit GasUnwrapped(msg.sender, amount);
+ emit GasUnwrapped(from, receiver, amount);
}Option 2: Make it directly callable by users (remove onlyWatcher, burn and send to msg.sender):
-function unwrapFromGas(uint256 amount, address receiver) external onlyWatcher {
+function unwrapFromGas(uint256 amount) external {
if (gasAccountToken__().balanceOf(msg.sender) < amount) revert InsufficientGasAvailable();
// Burn tokens from sender
gasAccountToken__().burn(msg.sender, amount);
- bool success = gasVault__().withdraw(receiver, amount);
+ bool success = gasVault__().withdraw(msg.sender, amount);
if (!success) revert InsufficientBalance();
emit GasUnwrapped(msg.sender, amount);
}Verify which pattern matches the intended design.
🤖 Prompt for AI Agents
In contracts/evmx/fees/GasAccountManager.sol around lines 82 to 93,
unwrapFromGas currently burns SGAS from msg.sender (the watcher) which is
incorrect; update the function to either (Option A) burn tokens from the
receiver/user account (e.g., call gasAccountToken__().burn(receiver, amount))
while keeping onlyWatcher so the watcher triggers unwrapping for the user, or
(Option B) remove the onlyWatcher modifier so users call unwrapFromGas
themselves and burn/send to msg.sender (burn(msg.sender) and withdraw to
msg.sender); choose the pattern that matches the contract design and update the
burn target, the withdraw recipient, and the event accordingly, and add/adjust
access control checks and error messages to reflect the chosen flow.
| function depositFromChain(bytes memory payload_) external onlyWatcher { | ||
| // Decode payload: (chainSlug, token, receiver, gasAmount, nativeAmount) | ||
| ( | ||
| uint32 chainSlug, | ||
| address token, | ||
| address depositTo, | ||
| uint256 gasAmount, | ||
| uint256 nativeAmount | ||
| ) = abi.decode(payload_, (uint32, address, address, uint256, uint256)); | ||
|
|
||
| tokenOnChainBalances[chainSlug][toBytes32Format(token)] += gasAmount + nativeAmount; | ||
|
|
||
| // Mint tokens to the user | ||
| gasAccountToken__().mint(depositTo, gasAmount); | ||
| if (nativeAmount > 0) { | ||
| // if native transfer fails, add to gas | ||
| bool success = gasVault__().withdraw(depositTo, nativeAmount); | ||
|
|
||
| if (!success) { | ||
| // Convert failed native amount to gas | ||
| gasAccountToken__().mint(depositTo, nativeAmount); | ||
| gasAmount += nativeAmount; | ||
| nativeAmount = 0; | ||
| } | ||
| } | ||
|
|
||
| emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add zero address validation for depositTo parameter.
The function should validate that depositTo is not the zero address to prevent tokens from being minted to an inaccessible address.
function depositFromChain(bytes memory payload_) external onlyWatcher {
// Decode payload: (chainSlug, token, receiver, gasAmount, nativeAmount)
(
uint32 chainSlug,
address token,
address depositTo,
uint256 gasAmount,
uint256 nativeAmount
) = abi.decode(payload_, (uint32, address, address, uint256, uint256));
+ if (depositTo == address(0)) revert InvalidReceiver();
tokenOnChainBalances[chainSlug][toBytes32Format(token)] += gasAmount + nativeAmount;📝 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 depositFromChain(bytes memory payload_) external onlyWatcher { | |
| // Decode payload: (chainSlug, token, receiver, gasAmount, nativeAmount) | |
| ( | |
| uint32 chainSlug, | |
| address token, | |
| address depositTo, | |
| uint256 gasAmount, | |
| uint256 nativeAmount | |
| ) = abi.decode(payload_, (uint32, address, address, uint256, uint256)); | |
| tokenOnChainBalances[chainSlug][toBytes32Format(token)] += gasAmount + nativeAmount; | |
| // Mint tokens to the user | |
| gasAccountToken__().mint(depositTo, gasAmount); | |
| if (nativeAmount > 0) { | |
| // if native transfer fails, add to gas | |
| bool success = gasVault__().withdraw(depositTo, nativeAmount); | |
| if (!success) { | |
| // Convert failed native amount to gas | |
| gasAccountToken__().mint(depositTo, nativeAmount); | |
| gasAmount += nativeAmount; | |
| nativeAmount = 0; | |
| } | |
| } | |
| emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount); | |
| } | |
| function depositFromChain(bytes memory payload_) external onlyWatcher { | |
| // Decode payload: (chainSlug, token, receiver, gasAmount, nativeAmount) | |
| ( | |
| uint32 chainSlug, | |
| address token, | |
| address depositTo, | |
| uint256 gasAmount, | |
| uint256 nativeAmount | |
| ) = abi.decode(payload_, (uint32, address, address, uint256, uint256)); | |
| if (depositTo == address(0)) revert InvalidReceiver(); | |
| tokenOnChainBalances[chainSlug][toBytes32Format(token)] += gasAmount + nativeAmount; | |
| // Mint tokens to the user | |
| gasAccountToken__().mint(depositTo, gasAmount); | |
| if (nativeAmount > 0) { | |
| // if native transfer fails, add to gas | |
| bool success = gasVault__().withdraw(depositTo, nativeAmount); | |
| if (!success) { | |
| // Convert failed native amount to gas | |
| gasAccountToken__().mint(depositTo, nativeAmount); | |
| gasAmount += nativeAmount; | |
| nativeAmount = 0; | |
| } | |
| } | |
| emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/GasAccountManager.sol around lines 99 to 126, add a
validation at the start of depositFromChain to ensure depositTo is not the zero
address; if depositTo == address(0) revert with a clear error (e.g., "Deposit to
zero address"); place this check before any state updates, minting, or transfers
(i.e., before updating tokenOnChainBalances and calling mint/withdraw) so
funds/tokens cannot be sent to an inaccessible address.
| function setGasStation(uint32 chainSlug_, bytes32 gasStation_) external onlyOwner { | ||
| gasStations[chainSlug_] = gasStation_; | ||
| emit GasStationSet(chainSlug_, gasStation_); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add zero address validation for setGasStation.
Setting a gas station to bytes32(0) would cause _getGasStationAddress to revert with InvalidChainSlug() instead of clearly indicating a configuration error.
function setGasStation(uint32 chainSlug_, bytes32 gasStation_) external onlyOwner {
+ if (gasStation_ == bytes32(0)) revert InvalidChainSlug();
gasStations[chainSlug_] = gasStation_;
emit GasStationSet(chainSlug_, gasStation_);
}🤖 Prompt for AI Agents
In contracts/evmx/fees/GasAccountManager.sol around lines 227 to 230, the
setGasStation function should validate the provided gasStation_ is not
bytes32(0) to avoid misconfiguration; add a check at the top of the function
that reverts when gasStation_ == bytes32(0) (use a custom error or require with
a clear message like "InvalidGasStation") before assigning and emitting the
event so callers get an explicit configuration error instead of downstream
InvalidChainSlug().
| function rescueFunds(address token_, address rescueTo_, uint256 amount_) external onlyWatcher { | ||
| RescueFundsLib._rescueFunds(token_, rescueTo_, amount_); | ||
| } |
There was a problem hiding this comment.
Critical: rescueFunds access control inconsistency.
This contract uses onlyWatcher for rescueFunds, while GasStation and GasAccountToken use onlyOwner or RESCUE_ROLE. Allowing the watcher to rescue funds poses a significant security risk, as the watcher could drain the contract.
-function rescueFunds(address token_, address rescueTo_, uint256 amount_) external onlyWatcher {
+function rescueFunds(address token_, address rescueTo_, uint256 amount_) external onlyRole(RESCUE_ROLE) {
RescueFundsLib._rescueFunds(token_, rescueTo_, amount_);
}Or use onlyOwner to match GasAccountToken:
-function rescueFunds(address token_, address rescueTo_, uint256 amount_) external onlyWatcher {
+function rescueFunds(address token_, address rescueTo_, uint256 amount_) external onlyOwner {
RescueFundsLib._rescueFunds(token_, rescueTo_, amount_);
}Committable suggestion skipped: line range outside the PR's diff.
| console.log("Sender balance in wei:", balance); | ||
| console.log("App Gateway:", appGateway); | ||
| console.log("Fees Plug:", address(feesPlug)); | ||
| console.log("Fees Plug:", address(gasStation)); |
There was a problem hiding this comment.
Update log message to match new terminology.
The log message still says "Fees Plug:" but should say "Gas Station:" to align with the renamed variable and broader migration.
Apply this diff:
- console.log("Fees Plug:", address(gasStation));
+ console.log("Gas Station:", address(gasStation));📝 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.
| console.log("Fees Plug:", address(gasStation)); | |
| console.log("Gas Station:", address(gasStation)); |
🤖 Prompt for AI Agents
In script/helpers/DepositGasAndNative.s.sol around line 31, the console.log
message still uses the old terminology "Fees Plug:"; update that string to "Gas
Station:" so the log matches the renamed variable (gasStation) and current
terminology — replace the logged label only, keeping the address(gasStation)
expression unchanged.
| // source .env && forge script script/helpers/DepositCreditAndNative.s.sol --broadcast --skip-simulation | ||
| contract DepositCredit is Script { | ||
|
|
||
| // source .env && forge script script/helpers/DepositForGasAndNative.s.sol --broadcast --skip-simulation |
There was a problem hiding this comment.
Update script path in comment to match actual filename.
The comment references DepositForGasAndNative.s.sol but the actual filename is DepositGasMainnet.s.sol. Update the comment for consistency.
Apply this diff:
-// source .env && forge script script/helpers/DepositForGasAndNative.s.sol --broadcast --skip-simulation
+// source .env && forge script script/helpers/DepositGasMainnet.s.sol --broadcast --skip-simulation📝 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.
| // source .env && forge script script/helpers/DepositForGasAndNative.s.sol --broadcast --skip-simulation | |
| // source .env && forge script script/helpers/DepositGasMainnet.s.sol --broadcast --skip-simulation |
🤖 Prompt for AI Agents
In script/helpers/DepositGasMainnet.s.sol around line 10, the shell comment
references the wrong script filename (`DepositForGasAndNative.s.sol`); update
that comment to reference the actual file `DepositGasMainnet.s.sol` (keeping the
rest of the command flags intact) so the inline usage instruction matches the
file name.
| console.log("Fees Manager:", address(feesManager)); | ||
| console.log("totalCredits fees:", totalCredits); | ||
| console.log("blockedCredits fees:", blockedCredits); | ||
| console.log("Fees Manager:", address(gasAccountManager)); |
There was a problem hiding this comment.
Fix the outdated log label.
The label "Fees Manager:" is inconsistent with the gas-based migration. It should reference "Gas Account Manager:".
- console.log("Fees Manager:", address(gasAccountManager));
+ console.log("Gas Account Manager:", address(gasAccountManager));📝 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.
| console.log("Fees Manager:", address(gasAccountManager)); | |
| console.log("Gas Account Manager:", address(gasAccountManager)); |
🤖 Prompt for AI Agents
In script/helpers/TransferRemainingGas.s.sol around line 32, the console.log
label reads "Fees Manager:" which is outdated for this gas-focused migration;
update the string literal to "Gas Account Manager:" so the log correctly
reflects the gas account manager address (i.e., change the log label while
leaving the address argument unchanged).
| console.log("App Gateway:", appGateway); | ||
| console.log("Fees Manager:", address(gasAccountToken)); | ||
| console.log("total gas fees:", totalGasFees); | ||
| console.log("payloadEscrow gas fees:", payloadEscrowGasFees); |
There was a problem hiding this comment.
Undefined variable breaks compilation. Line 27 logs payloadEscrowGasFees, but that identifier is never declared. This should log payloadEscrow; otherwise forge script fails to compile. Apply this fix:
- console.log("payloadEscrow gas fees:", payloadEscrowGasFees);
+ console.log("payloadEscrow gas fees:", payloadEscrow);📝 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.
| console.log("payloadEscrow gas fees:", payloadEscrowGasFees); | |
| console.log("payloadEscrow gas fees:", payloadEscrow); |
🤖 Prompt for AI Agents
In script/helpers/WithdrawRemainingGas.s.sol around line 27, the code logs an
undefined identifier payloadEscrowGasFees which breaks compilation; change the
console.log to reference the existing payloadEscrow variable (or the correct
declared variable) so it reads the declared identifier instead of
payloadEscrowGasFees, then re-run forge compile to verify the error is resolved.
| 1, // gasAccountManager.nonces(user_), // todo | ||
| deadline | ||
| ) | ||
| ); | ||
| bytes32 digest = keccak256( | ||
| abi.encodePacked("\x19\x01", feesManager.DOMAIN_SEPARATOR(), structHash) | ||
| abi.encodePacked("\x19\x01", gasAccountToken.DOMAIN_SEPARATOR(), structHash) | ||
| ); | ||
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(userPrivateKey_, digest); | ||
|
|
||
| feesManager.permit(user_, appGateway_, value, deadline, v, r, s); | ||
| gasAccountToken.permit(user_, appGateway_, value, deadline, v, r, s); | ||
| assertEq( |
There was a problem hiding this comment.
Use the real permit nonce
This should sign with the actual nonce the token contract expects. Hard-coding 1 makes the EIP-2612 digest differ from what gasAccountToken.permit verifies (fresh accounts start at nonce 0), so the permit call reverts the moment this helper is used. Pull the live nonce from the token (or whichever contract now tracks it) before hashing.
- 1, // gasAccountManager.nonces(user_), // todo
+ gasAccountToken.nonces(user_),🤖 Prompt for AI Agents
In test/SetupTest.t.sol around lines 485 to 495, the EIP‑2612 permit digest is
being built with a hard-coded nonce of 1 which mismatches the contract's actual
nonce and causes permit to revert; replace the hard-coded nonce with the live
nonce queried from the token (e.g. call gasAccountToken.nonces(user_) or the
correct contract that tracks permit nonces) and use that value when computing
structHash/digest and when signing so vm.sign signs the correct digest the
contract will verify.
Summary by CodeRabbit
Release Notes
New Features
Updates