Surge gameplay iteration stage #205
Conversation
…dd callAp to forwarderSolana
…ts in WriterPrecompile
… contracts to be able to call solana
… .. token_) params from bytes32 to address - not used param which is problematic
…t it can do forwarder calls;
…irpt to set it; configure feesPlug solana as plug for feesManager gateway
WalkthroughIntroduces Solana blockchain integration throughout the protocol via new Ed25519 cryptographic verification, PDA derivation, and Solana-specific contract interactions. Refactors fees and credit management to support both EVM and Solana chains. Updates multiple contract interfaces and deployment configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ForwarderSolana
participant SolanaSignature
participant Ed25519
participant SolanaPda
Client->>ForwarderSolana: callSolana(payload, target, callerAppGateway)
ForwarderSolana->>ForwarderSolana: msgSender = callerAppGateway
ForwarderSolana->>ForwarderSolana: Check isAsyncModifierSet()
Note over ForwarderSolana: Path A: Signature Verification
ForwarderSolana->>SolanaSignature: verifyMessage(pubkey, r, s, msg)
SolanaSignature->>Ed25519: verify(k, r, s, m)
Ed25519->>Ed25519: Compute SHA-512(R, A, M)
Ed25519->>Ed25519: Unpack & compute elliptic-curve points
Ed25519->>Ed25519: Final comparison & masking
Ed25519-->>SolanaSignature: bool result
SolanaSignature-->>ForwarderSolana: Signature valid/invalid
Note over ForwarderSolana: Path B: PDA Derivation
ForwarderSolana->>SolanaPda: findProgramAddress(programId, seeds)
SolanaPda->>SolanaPda: Iterate bump: 255 → 0
SolanaPda->>SolanaPda: sha256(seeds || bump || programId || "ProgramDerivedAddress")
SolanaPda->>SolanaPda: Check result is OFF-curve
SolanaPda-->>ForwarderSolana: (pda, bump)
ForwarderSolana-->>Client: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
script/super-token-solana/EvmSolanaOnchainCalls.s.sol (1)
215-221: Enforce u64 width for Solana argument; cast explicitly.This should validate and cast order.srcAmount to uint64 to match "u64" type name. Without it, large values silently misencode and break Solana program expectations.
Apply this diff:
function buildMintTokenSolanaInstruction( EvmSolanaAppGateway.TransferOrderEvmToSolana memory order ) internal view returns (SolanaInstruction memory) { + // Ensure EVM value fits Solana u64 + require(order.srcAmount <= type(uint64).max, "srcAmount exceeds u64"); @@ - bytes[] memory functionArguments = new bytes[](1); - // TODO:GW: in watcher and transmitter we might need to convert this value if on Solana mint has different decimals, for now we assume that both are the same - functionArguments[0] = abi.encode(order.srcAmount); + bytes[] memory functionArguments = new bytes[](1); + // TODO: decimals alignment to be handled in watcher/transmitter if needed + functionArguments[0] = abi.encode(uint64(order.srcAmount));contracts/evmx/watcher/RequestHandler.sol (1)
283-295: Fix state mutation order and missing zero-address guard in getPrecompileFees._createRequest stores state at line 226 before the loop where _validateAndGetPrecompileData (a non-view, external call) executes at line 247. If the precompile reverts mid-request, _requestBatchIds is already mutated. Move the external call before storage writes, or restructure to defer all mutations until validation succeeds.
Also add a zero-address check to getPrecompileFees (line 119) matching the guard at _validateAndGetPrecompileData line 288:
function getPrecompileFees( bytes4 callType_, bytes memory precompileData_ ) external view returns (uint256) { if (address(precompiles[callType_]) == address(0)) revert InvalidCallType(); return precompiles[callType_].getPrecompileFees(precompileData_); }Without this guard, callers get an opaque low-level call revert instead of a domain-specific error. The precompile could also be a zero address if misconfigured, leading to silent failures.
contracts/evmx/helpers/ForwarderSolana.sol (1)
59-76: Critical: Unauthenticated caller spoofing via callerAppGateway.callSolana trusts the caller-provided callerAppGateway instead of msg.sender. Any EOA/contract can spoof a valid AppGateway, passing isAsyncModifierSet() and getOverrideParams(), and be queued as that gateway. This should bind callerAppGateway to msg.sender (or an allowlisted proxy) and validate inputs.
- function callSolana( + function callSolana( bytes memory solanaPayload, bytes32 target, address callerAppGateway ) external { @@ - // address msgSender = msg.sender; - address msgSender = callerAppGateway; + if (callerAppGateway == address(0)) revert InvalidOnChainAddress(); + // Bind the logical gateway to the actual caller to prevent spoofing. + if (callerAppGateway != msg.sender) revert AsyncModifierNotSet(); // or define a dedicated error e.g., InvalidCaller() + address msgSender = callerAppGateway;Add basic payload/target validation too:
+ if (solanaPayload.length == 0) revert InvalidOnChainAddress(); + if (target == bytes32(0)) revert InvalidOnChainAddress();If proxy callers are intended, maintain an allowlist in AddressResolver and check msg.sender against it, not arbitrary input.
hardhat-scripts/deploy/1.deploy.ts (1)
107-115: Missing await on getWatcherSigner().getWatcherSigner() is awaited elsewhere and likely returns a Promise. Without await, signer becomes a Promise, breaking uses like signer.address.
- const signer: Wallet = getWatcherSigner(); + const signer: Wallet = await getWatcherSigner();hardhat-scripts/deploy/3.configureChains.ts (1)
502-506: Bug: missing await on susdcContractId() breaks forwarder lookup.This returns a Promise and is used immediately, causing invalid argument errors downstream.
Apply:
- const susdcToken = contractInstance.susdcContractId(); + const susdcToken = await contractInstance.susdcContractId();contracts/evmx/watcher/precompiles/WritePrecompile.sol (4)
178-183: Fix misleading variable/comment in decode tuple.value is msg.value-equivalent; comment says switchboardId. Either capture the switchboardId into a variable or fix the comment to avoid confusion. This should not ship with misleading annotations.
- uint256 value, // switchboardId + uint256 value, + uint64 /* switchboardId */
306-358: Hardcoded Solana socket/transmitter is unsafe and environment-specific.Using hardcodedSocket and transmitterSolana bakes network addresses into the binary and breaks configurability, upgrades, and testability. This should be injected from configuration (e.g., configurations__(), storage setters) and validated per chainSlug. Also remove the unused transmitter_ parameter in the signature and call site to avoid confusion.
- function _createSolanaDigestParams( + function _createSolanaDigestParams( PayloadParams memory payloadParams_, Transaction memory transaction_, address appGateway_, - address /*transmitter_*/, + // transmitter and socket are injected via configuration bytes32 prevBatchDigestHash_, uint256 deadline_, uint256 gasLimit_, uint256 value_ - ) internal returns (DigestParams memory) { + ) internal returns (DigestParams memory) { @@ - // TODO: this is temporary, must be injected from function arguments - // bytes32 of Solana Socket address : 9vFEQ5e3xf4eo17WttfqmXmnqN3gUicrhFGppmmNwyqV - bytes32 hardcodedSocket = 0x84815e8ca2f6dad7e12902c39a51bc72e13c48139b4fb10025d94e7abea2969c; - // bytes32 of Solana transmitter address : pFCBP4bhqdSsrWUVTgqhPsLrfEdChBK17vgFM7TxjxQ - bytes32 transmitterSolana = 0x0c1a5886fe1093df9fc438c296f9f7275b7718b6bc0e156d8d336c58f083996d; + bytes32 socket = configurations__().sockets(transaction_.chainSlug); + bytes32 transmitterSolana = configurations__().transmitters(transaction_.chainSlug); @@ - hardcodedSocket, + socket,And update call site to drop the unused transmitter_ argument.
396-403: Doc/modifier mismatch.setExpiryTime says “Only callable by the contract owner” but uses onlyWatcher. Fix the comment or the modifier. This should be consistent.
224-231: Fix transmitter type divergence in WriteProofRequested event for Solana flows.The event emits
address transmitter_but the digest uses a different transmitter for Solana chains:
- Line 201 TODO explicitly states this is known: "we should use here Solana transmitter address"
- Line 310:
_createSolanaDigestParamsignores the transmitter parameter entirely- Lines 340, 346: Solana digest actually uses
transmitterSolana(bytes32), a hardcoded Solana address- Line 225: Event always emits the EVM address passed to the function
Off-chain consumers reading the event will see a different transmitter than what was used to compute the digest.
Change the event field to
bytes32and emit the correct transmitter for each chain, or add a Solana-specific event variant.
🧹 Nitpick comments (33)
.gitignore (1)
16-16: Consider whether.env.exampleor template files should be committed.The
.env.*pattern is overly broad—it ignores all environment files with extensions, including template/reference files like.env.exampleor.env.templatethat teams typically want to version-control for documentation.If the project needs to commit environment templates, either use a more specific pattern (e.g.,
.env.local*,.env.*.local) or add an exception rule:.env -.env.* +.env.* +!.env.exampleAlternatively, if all
.env.*files should be ignored regardless, this is fine—just verify that's intentional.test/ReturnValueSolanaTest.t.sol (1)
21-22: LGTM. Add length checks to harden the test.This should assert decoded sizes to catch schema drift early.
Example patch (contextual):
bytes[] memory parsedData = BorshDecoder.decodeGenericSchema(returnDataSchema, returnData); +assertEq(parsedData.length, 2, "schema items"); uint8[] memory transmitterSolanaArray = abi.decode(parsedData[0], (uint8[])); bytes memory transmitterSolana = BorshEncoder.packUint8Array(transmitterSolanaArray); +assertEq(transmitterSolana.length, 32, "transmitter len"); uint8[] memory returnValueArray = abi.decode(parsedData[1], (uint8[])); bytes memory returnValue = BorshEncoder.packUint8Array(returnValueArray); +assertEq(returnValue.length, 4, "return value len");Also applies to: 37-40
setupInfraContracts.sh (1)
22-24: Clarify the commented-out upgrade script.The UpgradeForwarderSolana.ts script is commented out. Either enable this step if it's required for the Solana integration, or remove it if it's not needed for this deployment.
If this is an optional upgrade path for future use, add a comment explaining when it should be enabled:
+# Optional: Uncomment to upgrade ForwarderSolana implementation +# Required after initial deployment when implementation changes # time npx hardhat run hardhat-scripts/deploy/UpgradeForwarderSolana.ts --no-compilecontracts/evmx/watcher/RequestHandler.sol (1)
115-121: Guard against unset precompile to avoid opaque zero‑address call reverts.getPrecompileFees should revert with InvalidCallType when precompiles[callType_] is unset.
Apply this diff:
function getPrecompileFees( bytes4 callType_, bytes memory precompileData_ ) external view returns (uint256) { - return precompiles[callType_].getPrecompileFees(precompileData_); + IPrecompile pre = precompiles[callType_]; + if (address(pre) == address(0)) revert InvalidCallType(); + return pre.getPrecompileFees(precompileData_); }script/super-token-solana/EvmSolanaOnchainCalls.s.sol (2)
309-344: Type descriptor vs. encoded value mismatch ([u64;100] vs uint256[100]).This should encode a uint64[100] (or update the descriptor). Keeping uint256 here contradicts "functionArgumentTypeNames[1] = "[u64;100]"".
Apply this diff:
- uint256[] memory array = new uint256[](100); - functionArguments[1] = abi.encode(array); + uint64[] memory array64 = new uint64[](100); + functionArguments[1] = abi.encode(array64);
33-37: Avoid hard‑coding mainnet slug and unconditional enable in scripts.This should be gated by env to prevent accidental mainnet state mutation when someone runs the script with real RPCs.
Apply this diff:
- // appGateway.setIsValidPlug(CHAIN_SLUG_SOLANA_MAINNET, solanaTargetProgramId); - appGateway.setIsValidPlugForSolana(true, CHAIN_SLUG_SOLANA_MAINNET, solana_plug_signer); + bool enable = vm.envBool("ENABLE_SET_VALID_PLUG"); + bytes32 chainSlug = vm.envBytes32("CHAIN_SLUG"); // e.g., CHAIN_SLUG_SOLANA_MAINNET + appGateway.setIsValidPlugForSolana(enable, chainSlug, solana_plug_signer); - // appGateway.setIsValidPlug(CHAIN_SLUG_SOLANA_MAINNET, solana_plug_signer); + // appGateway.setIsValidPlug(chainSlug, solana_plug_signer);contracts/evmx/watcher/borsh-serde/BorshDecoder.sol (1)
14-18: Validate full consumption to catch truncated/malformed payloads.This should assert there are no trailing bytes after decoding the declared fields. Add data.done() before returning.
Apply this diff:
for (uint256 i = 0; i < schema.valuesTypeNames.length; i++) { ... } - - return decodedParams; + data.done(); + return decodedParams;Also applies to: 101-106
contracts/evmx/helpers/solana-utils/SolanaSignature.sol (2)
1-2: Align SPDX license identifier with repo convention.This should use "GPL-3.0-only" like other contracts.
Apply this diff:
-// SPDX-License-Identifier: GPL-3.0 +// SPDX-License-Identifier: GPL-3.0-only
13-16: Inline return; drop the temporary variable.Saves a few bytes and reads cleaner.
Apply this diff:
- bool valid = Ed25519.verify(public_key, signature_r, signature_s, message); - - return valid; + return Ed25519.verify(public_key, signature_r, signature_s, message);contracts/evmx/helpers/solana-utils/Ed25519_pow.sol (2)
5-8: Document the addition chain and return semantics.This should state the exact addition chain used to compute 2^250−1 and prove p11 is v^11. Add a brief derivation and cite the standard chain to aid auditing and maintenance.
40-328: Highly unrolled squaring inflates code size; risk of exceeding contract size and high deployment cost.This unrolled chain issues hundreds of mulmod calls with repeated constants. It risks hitting the 24KB code size limit once linked into the consuming library/contract and significantly increases deployment gas. This should be collapsed into small helpers to square N times and to multiply when needed; the compiler will keep loops tight while shrinking bytecode.
Example approach:
+ function _squareN(uint256 x, uint256 n) private pure returns (uint256 r) { + r = x; + unchecked { + for (uint256 i = 0; i < n; ++i) r = mulmod(r, r, P); + } + }Then rewrite long runs like “a = mulmod(a,a,P)” repeated K times into “a = _squareN(a, K)”. Keep the exact addition chain but reduce bytecode and improve auditability. Consider the modexp precompile (0x05) for v^(2^250−1) if acceptable for gas/precision trade‑offs.
hardhat-scripts/deploy/1.deploy.ts (1)
522-536: Provider init is independent of Hardhat runtime; consider reusing the signer’s provider.getStorageAt via a separate JsonRpcProvider can drift from the active network. This should reuse deployUtils.signer.provider or ensure EVMX_RPC matches the current network.
test/BorshDecoderTest.t.sol (1)
684-701: Remove noisy console logs in stable tests.console.log/console.logBytes add clutter and slow CI. This should be gated behind a debug flag or removed in non-debug tests.
contracts/evmx/watcher/Configurations.sol (1)
121-136: Docstring vs access control mismatch.setIsValidPlug comment says “Only callable by the app gateway” but the guard is onlyWatcher. This should be corrected to avoid confusion.
test/apps/app-gateways/super-token/EvmSolanaAppGateway.sol (1)
123-126: Direct watcher setIsValidPlug call bypasses access checks on gateway side.This should remain limited to test scaffolding. In production, only the watcher owner/admin should configure validity. Consider protecting this function (onlyOwner) in tests or confining to Foundry scripts.
test/BorshEncoderTest.t.sol (1)
252-276: Use abi.encodePacked for fixed 32-byte arguments to match decoder expectations.Your comment explains the panic when using abi.encode; keeping raw 32 bytes or abi.encodePacked is correct for [u8;32]. Consider adding a small helper to avoid future mistakes.
hardhat-scripts/deploy/6.connect.ts (1)
1-1: Avoid ethers.utils in v6; support both v5 and v6.Using ethers.utils.hexZeroPad assumes v5. If the repo is on ethers v6, utils is removed. This should use zeroPadValue/hexlify for v6 or centralize in a helper.
Minimal compatibility wrapper:
-const solanaAppGatewayId = ethers.utils.hexZeroPad(appGatewayAddress, 32); +const zeroPad32 = (v: string) => + (ethers as any).utils?.hexZeroPad ? (ethers as any).utils.hexZeroPad(v, 32) : (ethers as any).zeroPadValue(v, 32); +const solanaAppGatewayId = zeroPad32(appGatewayAddress);Or better: use toBytes32FormatHexString everywhere (as above) and drop direct hexZeroPad calls.
Also applies to: 183-184, 230-233, 244-248, 258-263
contracts/evmx/fees/FeesManager.sol (1)
150-156: Emit events on Solana program ID changes.Administrative setters should emit events for observability and off-chain indexing.
function setFeesPlugSolanaProgramId(bytes32 feesPlugSolanaProgramId_) external onlyOwner { feesPlugSolanaProgramId = feesPlugSolanaProgramId_; + emit FeesPlugSolanaSet(feesPlugSolanaProgramId_); } function setSusdcSolanaProgramId(bytes32 susdcSolanaProgramId_) external onlyOwner { susdcSolanaProgramId = susdcSolanaProgramId_; + emit SusdcSolanaProgramSet(susdcSolanaProgramId_); }(Define the two events.)
contracts/evmx/helpers/solana-utils/program-pda/SUSDCPdas.sol (1)
12-17: Seed literals look fine; add a canonical-bump variant and doc the trailing “:” seeds.These PDAs depend on exact seed bytes (including the trailing colon). This should be documented to avoid silent mismatches with the on-chain Solana programs. Consider adding a validate helper that asserts the derived bump is canonical using SolanaPDA.validatePDAWithCanonicalBump to catch accidental noncanonical bumps in tests.
Also applies to: 19-24
hardhat-scripts/deploy/3.configureChains.ts (1)
207-213: Logging path also assumes hex input; fix to log base58→hex conversion.toBytes32Format(solanaSwitchboard) expects hex but you’ll pass base58. Use toBytes32FromBase58 for display too.
- console.log( - "solanaSwitchboard as bytes32: ", - Buffer.from(toBytes32Format(solanaSwitchboard)).toString("hex") - ); + const ssHex = toBytes32FromBase58(solanaSwitchboardB58); + console.log("solanaSwitchboard as bytes32:", ssHex);contracts/evmx/helpers/solana-utils/program-pda/FeesPlugPdas.sol (1)
30-39: Expose seed bytes intentionally; add unit tests asserting Solana parity.These helpers mirror Solana program logic and look correct. Add tests that cross-check against a reference off-chain PDA (same seeds, programId) to prevent regressions and ensure the trailing colon seed strings match the on-chain program.
Also applies to: 41-46
contracts/evmx/helpers/solana-utils/SolanaPda.sol (3)
6-13: False-positive secret scan; constants are public program IDs.These hex constants are base58-decoded program IDs, not secrets. Add a comment to silence future secret scans and include the original base58 strings for auditability.
-// TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA +// Token Program (base58): TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA — public constant, not a secret // ... -// TokenzQdBNbLqP5VEhdkAS6EPFLC1PHnBqCXEpPxuEb +// Token-2022 (base58): TokenzQdBNbLqP5VEhdkAS6EPFLC1PHnBqCXEpPxuEb — public constant, not a secret // ... -// ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL +// Associated Token (base58): ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL — public constant, not a secret
223-240: deriveTokenAtaAddress: OK; add a counterpart for Token-2022.Some mints use TOKEN_2022_PROGRAM_ID. Provide an overload to derive ATAs for Token-2022, or a flag to select program ID.
- function deriveTokenAtaAddress( - bytes32 solanaAddress, - bytes32 mint - ) internal pure returns (bytes32) { + function deriveTokenAtaAddress( + bytes32 solanaAddress, + bytes32 mint + ) internal pure returns (bytes32) { bytes[] memory seeds = new bytes[](3); // user solanaAddress seeds[0] = abi.encodePacked(solanaAddress); // token programId seeds[1] = abi.encodePacked(TOKEN_PROGRAM_ID); // token mint seeds[2] = abi.encodePacked(mint); (bytes32 pda /*uint8 bump*/, ) = SolanaPDA.findProgramAddress( ASSOCIATED_TOKEN_PROGRAM_ID, seeds ); return pda; } + + function deriveToken2022AtaAddress( + bytes32 solanaAddress, + bytes32 mint + ) internal pure returns (bytes32) { + bytes[] memory seeds = new bytes[](3); + seeds[0] = abi.encodePacked(solanaAddress); + seeds[1] = abi.encodePacked(TOKEN_2022_PROGRAM_ID); + seeds[2] = abi.encodePacked(mint); + (bytes32 pda, ) = SolanaPDA.findProgramAddress(ASSOCIATED_TOKEN_PROGRAM_ID, seeds); + return pda; + }
147-186: Micro-optimization: use unchecked increments in tight loops.These loops are hot for PDA derivation. Wrap the p++ style counters in unchecked to shave gas.
contracts/evmx/interfaces/IFeesManager.sol (1)
59-65: Clarify burn vs burnSolana semantics.burnSolana adds burnTokenMint_. This should be mirrored in docs/Natspec and validated at implementation (correct Solana mint, chain slug). If both functions are externally visible, add explicit revert messages for wrong chainSlug.
EventTopics.md (2)
348-351: Mark Solana debug events as non-contractual.SolanaDecodedInstruction / SolanaFunctionArgsPacked / SolanaInstructionInput are debug-only. This should be clearly labeled as temporary in docs and omitted from any public API commitments to avoid downstream coupling.
153-158: ForwarderSolana section needs cross-reference.Add a link/section that explains where ForwarderSolana is set and which chains/slugs it applies to, to reduce ambiguity during integration.
contracts/evmx/fees/Credit.sol (6)
310-312: balanceOf may underflow-revert if blockedCredits > balance.Returning super.balanceOf - blockedCredits can revert on view. This should return 0 in that edge case or enforce the invariant elsewhere. At minimum, add a safety check.
- return super.balanceOf(account) - blockedCredits[account]; + uint256 bal = super.balanceOf(account); + uint256 blocked = blockedCredits[account]; + return blocked > bal ? 0 : bal - blocked;
418-441: withdrawCredits: burn occurs before pool balance decrement checks.If tokenOnChainBalances underflows, the whole tx reverts and the burn is reverted, but this ordering obscures the failure mode. This should validate and decrement pool balance before burning, or compute and assert sufficiency first to surface precise errors.
171-175: setFeesPlugSolana: mainnet-only.This hardcodes CHAIN_SLUG_SOLANA_MAINNET in watcher__.setIsValidPlug. If devnet/testnet are used, add a setter for those slugs or document the expectation.
251-265: wrap: event and accounting ok, but document ETH funding assumptions.wrap mints credits and sends ETH to FeesPool. Document how FeesPool is funded and reconciled with native withdrawals to avoid pool insolvency when deposits/withdraws occur via watchers.
548-563: mintSusdcOnSolana: missing slippage/amount guards.Consider enforcing a max mint amount or policy via configuration to prevent accidental large mints if upstream passes wrong units.
715-719: convertToSolanaUint64 silently truncates decimals.This divides by 1e12 and truncates. If precision matters, add rounding or require divisibility by 1e12 to prevent silent value loss.
- uint256 scaledAmount = amount / 10 ** 12; + require(amount % (10 ** 12) == 0, "Amount not 6-decimal aligned"); + uint256 scaledAmount = amount / (10 ** 12);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (52)
.gitignore(1 hunks)Errors.md(1 hunks)EventTopics.md(4 hunks)contracts/evmx/fees/Credit.sol(14 hunks)contracts/evmx/fees/FeesManager.sol(3 hunks)contracts/evmx/helpers/ForwarderSolana.sol(2 hunks)contracts/evmx/helpers/solana-utils/Ed25519.sol(1 hunks)contracts/evmx/helpers/solana-utils/Ed25519_pow.sol(1 hunks)contracts/evmx/helpers/solana-utils/Sha512.sol(1 hunks)contracts/evmx/helpers/solana-utils/SolanaPda.sol(1 hunks)contracts/evmx/helpers/solana-utils/SolanaSignature.sol(1 hunks)contracts/evmx/helpers/solana-utils/program-pda/FeesPlugPdas.sol(1 hunks)contracts/evmx/helpers/solana-utils/program-pda/SUSDCPdas.sol(1 hunks)contracts/evmx/interfaces/IConfigurations.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(3 hunks)contracts/evmx/interfaces/IPrecompile.sol(1 hunks)contracts/evmx/watcher/Configurations.sol(2 hunks)contracts/evmx/watcher/RequestHandler.sol(2 hunks)contracts/evmx/watcher/borsh-serde/BorshDecoder.sol(3 hunks)contracts/evmx/watcher/borsh-serde/BorshEncoder.sol(2 hunks)contracts/evmx/watcher/borsh-serde/BorshUtils.sol(2 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(5 hunks)contracts/utils/common/Structs.sol(1 hunks)deployments/dev_addresses.json(1 hunks)deployments/dev_verification.json(1 hunks)deployments/prod_verification.json(1 hunks)deployments/stage_verification.json(7 hunks)foundry.toml(1 hunks)hardhat-scripts/config/config.ts(2 hunks)hardhat-scripts/constants/feeConstants.ts(1 hunks)hardhat-scripts/deploy/1.deploy.ts(3 hunks)hardhat-scripts/deploy/2.roles.ts(1 hunks)hardhat-scripts/deploy/3.configureChains.ts(7 hunks)hardhat-scripts/deploy/6.connect.ts(4 hunks)hardhat-scripts/deploy/9.setupTransmitter.ts(1 hunks)hardhat-scripts/deploy/UpgradeForwarderSolana.ts(1 hunks)hardhat-scripts/deploy/WhitelistFeesReceiver.ts(1 hunks)hardhat-scripts/test/chainTest.ts(1 hunks)hardhat-scripts/utils/deployUtils.ts(3 hunks)hardhat-scripts/verify/verify.ts(1 hunks)hardhat.config.ts(4 hunks)lib/forge-std(1 hunks)lib/solady(1 hunks)package.json(2 hunks)script/super-token-solana/EvmSolanaOnchainCalls.s.sol(9 hunks)setupInfraContracts.sh(1 hunks)src/enums.ts(1 hunks)test/BorshDecoderTest.t.sol(40 hunks)test/BorshEncoderTest.t.sol(2 hunks)test/DigestTest.t.sol(1 hunks)test/ReturnValueSolanaTest.t.sol(2 hunks)test/apps/app-gateways/super-token/EvmSolanaAppGateway.sol(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
hardhat.config.ts (2)
hardhat-scripts/s3Config/buildConfig.ts (1)
getChainConfig(51-72)src/chain-enums/chainSlugToId.ts (1)
ChainSlugToId(4-83)
hardhat-scripts/deploy/UpgradeForwarderSolana.ts (7)
hardhat-scripts/constants/types.ts (1)
DeploymentAddresses(3-5)hardhat-scripts/utils/address.ts (1)
getAddresses(8-27)hardhat-scripts/config/config.ts (2)
mode(10-12)EVMX_CHAIN_ID(181-181)src/types.ts (1)
ChainAddressesObj(18-33)hardhat-scripts/utils/sign.ts (1)
getWatcherSigner(11-14)hardhat-scripts/utils/deployUtils.ts (4)
DeployParams(32-37)getOrDeploy(39-78)getInstance(131-135)storeAddresses(168-195)hardhat-scripts/utils/overrides.ts (1)
overrides(94-105)
hardhat-scripts/deploy/6.connect.ts (4)
hardhat-scripts/deploy/1.deploy.ts (1)
superTokenSolanaAddress32Bytes(51-54)hardhat-scripts/constants/types.ts (2)
AppGatewayConfig(14-21)DeploymentAddresses(3-5)hardhat-scripts/utils/address.ts (1)
getAddresses(8-27)hardhat-scripts/config/config.ts (2)
mode(10-12)EVMX_CHAIN_ID(181-181)
hardhat-scripts/deploy/3.configureChains.ts (5)
hardhat-scripts/config/config.ts (1)
EVMX_CHAIN_ID(181-181)hardhat-scripts/constants/constants.ts (2)
CCTP_SWITCHBOARD_TYPE(10-10)FAST_SWITCHBOARD_TYPE(9-9)hardhat-scripts/utils/address.ts (2)
toBytes32Format(58-65)toBytes32FormatHexString(42-56)hardhat-scripts/utils/deployUtils.ts (2)
updateContractSettings(348-387)getInstance(131-135)hardhat-scripts/utils/sign.ts (1)
getWatcherSigner(11-14)
🪛 Gitleaks (8.28.0)
contracts/evmx/helpers/solana-utils/SolanaPda.sol
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (32)
deployments/prod_verification.json (1)
131-131: Formatting consistency improvement.Inlining the Socket constructor arguments for chain 5000 matches the existing single-line format used for Socket entries on chains 169 and 747. No data changes; semantic content remains identical.
lib/solady (1)
1-1: Verify solady update addresses known security vulnerability.The new commit
836c169fis valid, but a critical vulnerability exists in solady: CVE-2025-53638 documents a lack-of-extcodesize validation issue in ERC4337Factory / silent initialization failures; upgrade to v0.1.24+ recommended.Confirm: (1) does commit
836c169finclude the security fix for CVE-2025-53638, (2) was the old commit vulnerable, and (3) does the codebase use ERC4337Factory or related components that require this update?lib/forge-std (1)
1-1: No issues—forge-std submodule update is justified and necessary.Verification confirms forge-std is actively used across multiple test files including
ReturnValueSolanaTest.t.sol,DigestTest.t.sol,BorshEncoderTest.t.sol, andBorshDecoderTest.t.sol—all directly supporting the Solana integration work in this PR. The update is not spurious; it's required test infrastructure for the cryptographic and protocol changes being introduced.package.json (2)
12-12: Patch version bump aligns with iterative Solana integration.The version bump from 1.1.48 to 1.1.49 is appropriate for this PR's scope. Since the package is in active development (alpha), patch increments are standard for feature and protocol iterations.
35-35: Verify package version constraint and review @socket.tech/socket-protocol-common release notes manually.Version discrepancy detected: package.json specifies 1.1.46, but yarn.lock contains @socket.tech/[email protected]. Confirm which version is intended—1.2.46 is a minor version bump (not a patch) and has higher breaking-change risk.
No public changelog exists for this package. Review the package release notes directly on npm or the Socket repository for any Solana PDA derivation, message type schema, or event signature changes. The codebase has extensive Solana integration (ForwarderSolana, SolanaPDA library, Borsh encoding), so confirm Solana-specific protocol updates don't conflict with existing Credit.sol, FeesManager.sol, or WritePrecompile.sol implementations.
contracts/utils/common/Structs.sol (1)
262-262: No-op formatting.Fine to keep; no behavioral impact.
test/DigestTest.t.sol (1)
28-28: Formatting-only change.No impact. Keep as is.
deployments/stage_verification.json (1)
56-56: Formatting change verified. No schema violations.The array compaction preserves the required structure—Socket entries maintain the
[chainId, address, "EVMX"]format across all updated lines (56, 114, 294, 351, 407, 465). The schema check passes.hardhat-scripts/constants/feeConstants.ts (1)
345-345: No action required. Ignore the guard suggestion.The guard is unnecessary because
modeis typed asDeploymentMode(from the castprocess.env.DEPLOYMENT_MODE as DeploymentMode | DeploymentMode.DEV), and all fourDeploymentModevalues—LOCAL, DEV, STAGE, PROD—are already mapped in thefeePoolsobject (lines 345–348). TypeScript enforces thatfeePools[mode]can never be undefined given the type constraints. Adding a guard would be defensive coding that masks the actual guarantee the type system provides.The address update at line 345 is verified and consistently referenced across the codebase (foundry.toml, dev_addresses.json, and the new LOCAL mapping).
Likely an incorrect or invalid review comment.
hardhat-scripts/test/chainTest.ts (1)
9-30: Test chain scope aligns with DEV configuration.The reduction to ARBITRUM_SEPOLIA and OPTIMISM_SEPOLIA matches the DEV mode chain list in config.ts, ensuring test coverage focuses on actively configured chains.
Errors.md (1)
19-25: Error documentation added for ForwarderSolana.Clear documentation of the new error signatures for Solana integration.
hardhat-scripts/config/config.ts (2)
43-65: DEV chain scope reduced for focused Solana iteration.The chain list reduction to ARBITRUM_SEPOLIA and OPTIMISM_SEPOLIA aligns with the test configuration changes and the PR's focus on Solana gameplay iteration.
35-35: No issues found. The evmChainId migration is complete and consistent.The old LOCAL chain ID (7625382) has zero references in the codebase. The new chain ID (14323) is correctly applied across LOCAL, DEV, and STAGE configs in
hardhat-scripts/config/config.tsand synchronized in all deployment JSON files. No hardcoded references to the old ID remain, and the change poses no risk to deployment targets.deployments/dev_verification.json (1)
1-5: Verification data reduced to match DEV chain scope.The minimal entries for chains 14323 (EVMX), 421614 (ARBITRUM_SEPOLIA), and 11155420 (OPTIMISM_SEPOLIA) align with the reduced DEV mode chain configuration.
foundry.toml (1)
20-45: Label updates align with Solana integration.The address label mappings correctly reflect the new ForwarderSolana contract architecture. The changes are consistent with the deployment addresses in dev_addresses.json.
deployments/dev_addresses.json (1)
1-56: Deployment addresses updated for Solana integration.The address updates across multiple networks (14323, 421614, 11155420) are consistent with the ForwarderSolana deployment. All new contract addresses follow the expected format.
src/enums.ts (1)
81-81: Enum addition follows established patterns.The ForwarderSolanaImpl enum entry is consistent with other implementation contract naming conventions in the codebase.
hardhat-scripts/deploy/9.setupTransmitter.ts (1)
26-27: Credit deposit for fees manager aligns with Solana integration.Ensuring the fees manager contract has sufficient credits is a logical step given the new Solana forwarding capabilities. This follows the same pattern used for the transmitter address.
contracts/evmx/watcher/borsh-serde/BorshUtils.sol (1)
23-29: Formatting improvements enhance readability.The multi-line formatting for memory calculations and inline comments for character token matching improve code readability without affecting functionality.
Also applies to: 106-132
contracts/evmx/interfaces/IConfigurations.sol (1)
11-16: All implementations and consumers are correctly synchronized with the interface change.The breaking change from
viewto mutable has been properly implemented:
- Interface (
IConfigurations.sol:11-16) and implementation (Configurations.sol:159-171) both showexternal(view modifier removed)- Event emission is correctly placed in the implementation (line 167:
emit VerifyGateway)- All callers (5 test locations in
Watcher.t.sol+ 1 production call inWritePrecompile.sol:121) use standard function calls, which are compatible with mutable functions- No static calls exist that would break—zero instances of
staticCallorcallStaticin the codebaseThe change is complete and requires no further action.
contracts/evmx/helpers/ForwarderSolana.sol (1)
81-93: Queue context integrity.watcher__().queue(queueParams, msgSender) relies on msgSender for downstream auth (Configurations.verifyConnections). This should only be a verified gateway address (post the fix above). Also consider emitting an event with chainSlug/target/appGateway to aid tracing.
hardhat-scripts/deploy/1.deploy.ts (3)
51-55: Name clarifies intent. LGTM.Renaming to superTokenSolanaAddress32Bytes improves clarity for Solana target.
175-187: Propagating possibly-zero ForwarderSolana into FeesManager.If deployment failed, FeesManager will be initialized with AddressZero. This should guard and fail fast (or skip FeesManager) to avoid configuring unusable state.
- deployUtils = await deployContractWithProxy( + if (forwarderSolanaAddress === ethers.constants.AddressZero) { + throw new Error("ForwarderSolana not deployed; aborting FeesManager deployment"); + } + deployUtils = await deployContractWithProxy(
1-5: No v5/v6 mix detected; codebase is consistently ethers v5.The project uses ethers 5.6.6 with @nomiclabs/hardhat-ethers 2.2.3. All imports follow v5 conventions:
formatEtherfrom "ethers/lib/utils",ethers.providers.JsonRpcProvider, andethers.utils.*patterns are correct and consistent throughout the codebase. The concern about mixed versions does not apply here.Likely an incorrect or invalid review comment.
test/BorshDecoderTest.t.sol (1)
774-816: Good coverage for envelope + inner payload decoding.Validates Vec envelope then u64 triplet. Clear assertions.
contracts/evmx/watcher/Configurations.sol (1)
160-171: verifyConnections changed to non-view; confirm callers and gas expectations.Switching to external (non‑view) impacts call sites and gas budgets. Ensure watcher__().queue callers don’t assume staticcall, and update interfaces.
test/apps/app-gateways/super-token/EvmSolanaAppGateway.sol (2)
135-141: Binding to ForwarderSolana.callerAppGateway is correct.Passing address(this) matches the new signature and helps bind the logical caller. This assumes ForwarderSolana enforces callerAppGateway == msg.sender.
270-279: Event-only “increase” is fine for testing.No state other than counter; good for signal.
test/BorshEncoderTest.t.sol (1)
278-295: Instruction args encoding smoke test looks good.encodeFunctionArgs(instruction) coverage is useful; keep payload in a fixture to declutter the test.
contracts/evmx/helpers/solana-utils/SolanaPda.sol (1)
70-141: Verification successful: all three program ID constants match their base58 encodings.The constants in SolanaPda.sol are correct:
TOKEN_PROGRAM_IDat line 7 ✓TOKEN_2022_PROGRAM_IDat line 9 ✓ASSOCIATED_TOKEN_PROGRAM_IDat line 11 ✓All decoded values match their hex representations exactly. The code is sound.
contracts/evmx/interfaces/IFeesManager.sol (1)
8-13: Inconsistency is intentional and correct—do not align token types.The bytes32 vs address difference reflects proper cross-chain vs on-chain distinction:
deposit(bytes32 token_)is a cross-chain bridge function that stores chain-specific token identifiers intokenOnChainBalances[chainSlug_][onChainTokenAddress_]withdrawCredits(address token_)is an on-chain EVM function that works with actual EVM token addressesThis design is sound. The pattern applies across Solana functions too:
withdrawCreditsSolana,burnSolanaall usebytes32for off-chain identifiers.The deposit implementation includes a TODO noting the parameter may be unused, but aligning it to
addresswould break cross-chain functionality. Instead, add brief interface documentation explaining the bytes32/address split reflects chain scope.Likely an incorrect or invalid review comment.
contracts/evmx/fees/Credit.sol (1)
93-101: Storage layout concern is partially inaccurate; gap math discrepancy needs clarification.The review lists "(forwarderSolana, susdcSolanaProgramId, feesPlugSolanaProgramId, whitelists, maps)" as new vars shrinking the gap. However:
- Only 3 vars are new (forwarderSolana, susdcSolanaProgramId, feesPlugSolanaProgramId)
- whitelistedEvmxReceivers and maxFeesPerChainSlug already existed at slot 59
Additionally, the gap math doesn't align: the comment reserves slots [60-107] (48 slots), but
_gap_afteris only 44 slots. With 3 new vars consuming 3 slots, the remaining gap should be 45 slots if the original was 48—the 1-slot discrepancy suggests a documentation error or alignment issue.To verify storage collision risk, compare the prior deployed contract's storage layout against the current version. This requires either a storage layout snapshot from the previous release or running
forge inspectagainst both versions—neither is available in the repo.
| // In deployment script add calling this for Solana chain slug | ||
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcContractId_) external onlyOwner { | ||
| if (chainSlug_ != CHAIN_SLUG_SOLANA_MAINNET) { | ||
| forwarderAddresses[susdcContractId][chainSlug_] = asyncDeployer__() | ||
| .getOrDeployForwarderContract(susdcContractId_, chainSlug_); | ||
| _setValidPlug(true, chainSlug_, susdcContractId); | ||
| } else { | ||
| // TODO: this is not entirely right, this method should be called setSusdcPlug() as SUSDC token on Solana is a separate thing that is managed by SusdcProgram plug | ||
| susdcSolanaProgramId = susdcContractId_; | ||
| watcher__().setIsValidPlug(true, chainSlug_, susdcContractId_); | ||
| } | ||
|
|
||
| emit SusdcTokenSet(chainSlug_, susdcContractId_); | ||
| } |
There was a problem hiding this comment.
Wrong identifier used and state not updated in setSusdcToken.
You index forwarderAddresses and setValidPlug with susdcContractId (state) while passing susdcContractId (param), and you never set susdcContractId for non-Solana chains. This should use the same id and update state.
function setSusdcToken(uint32 chainSlug_, bytes32 susdcContractId_) external onlyOwner {
- if (chainSlug_ != CHAIN_SLUG_SOLANA_MAINNET) {
- forwarderAddresses[susdcContractId][chainSlug_] = asyncDeployer__()
- .getOrDeployForwarderContract(susdcContractId_, chainSlug_);
- _setValidPlug(true, chainSlug_, susdcContractId);
+ if (chainSlug_ != CHAIN_SLUG_SOLANA_MAINNET) {
+ susdcContractId = susdcContractId_;
+ forwarderAddresses[susdcContractId][chainSlug_] = asyncDeployer__()
+ .getOrDeployForwarderContract(susdcContractId, chainSlug_);
+ _setValidPlug(true, chainSlug_, susdcContractId);
} else {
// ...
susdcSolanaProgramId = susdcContractId_;
watcher__().setIsValidPlug(true, chainSlug_, susdcContractId_);
}
emit SusdcTokenSet(chainSlug_, susdcContractId_);
}📝 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.
| // In deployment script add calling this for Solana chain slug | |
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcContractId_) external onlyOwner { | |
| if (chainSlug_ != CHAIN_SLUG_SOLANA_MAINNET) { | |
| forwarderAddresses[susdcContractId][chainSlug_] = asyncDeployer__() | |
| .getOrDeployForwarderContract(susdcContractId_, chainSlug_); | |
| _setValidPlug(true, chainSlug_, susdcContractId); | |
| } else { | |
| // TODO: this is not entirely right, this method should be called setSusdcPlug() as SUSDC token on Solana is a separate thing that is managed by SusdcProgram plug | |
| susdcSolanaProgramId = susdcContractId_; | |
| watcher__().setIsValidPlug(true, chainSlug_, susdcContractId_); | |
| } | |
| emit SusdcTokenSet(chainSlug_, susdcContractId_); | |
| } | |
| // In deployment script add calling this for Solana chain slug | |
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcContractId_) external onlyOwner { | |
| if (chainSlug_ != CHAIN_SLUG_SOLANA_MAINNET) { | |
| susdcContractId = susdcContractId_; | |
| forwarderAddresses[susdcContractId][chainSlug_] = asyncDeployer__() | |
| .getOrDeployForwarderContract(susdcContractId, chainSlug_); | |
| _setValidPlug(true, chainSlug_, susdcContractId); | |
| } else { | |
| // TODO: this is not entirely right, this method should be called setSusdcPlug() as SUSDC token on Solana is a separate thing that is managed by SusdcProgram plug | |
| susdcSolanaProgramId = susdcContractId_; | |
| watcher__().setIsValidPlug(true, chainSlug_, susdcContractId_); | |
| } | |
| emit SusdcTokenSet(chainSlug_, susdcContractId_); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 156-169, the function
setSusdcToken mixes the parameter susdcContractId_ with the state variable
susdcContractId and never updates the state for non-Solana chains; change the
forwarderAddresses index and the _setValidPlug call to use the parameter
susdcContractId_, and add an assignment to update the contract state
(susdcContractId = susdcContractId_) for the non-Solana branch so the stored id
matches what you register; keep the existing Solana branch behavior
(susdcSolanaProgramId) unchanged.
| function deposit( | ||
| uint32 chainSlug_, | ||
| // TODO: token will have to be bytes32 (for solana tokens) - this is the address of token on native chain (could be Solana) | ||
| address token_, | ||
| address depositTo_, | ||
| bytes32 onChainTokenAddress_, | ||
| address depositToEvmxAddress_, | ||
| uint256 nativeAmount_, | ||
| uint256 creditAmount_, | ||
| bytes memory data_ | ||
| ) external override onlyWatcher { | ||
| if (!whitelistedReceivers[depositTo_]) revert InvalidReceiver(); | ||
| tokenOnChainBalances[chainSlug_][token_] += creditAmount_ + nativeAmount_; | ||
| if (!whitelistedEvmxReceivers[depositToEvmxAddress_]) revert InvalidReceiver(); | ||
| tokenOnChainBalances[chainSlug_][onChainTokenAddress_] += creditAmount_ + nativeAmount_; | ||
|
|
||
| // Mint tokens to the some evmx accounting contract | ||
| _mint(depositTo_, creditAmount_); | ||
| _mint(depositToEvmxAddress_, creditAmount_); | ||
|
|
||
| if (nativeAmount_ > 0) { | ||
| // TODO: ask: what are native tokens in this context ? - native to real blockchains or to EVMx itself ? | ||
| // This is taking credit tokens and sending them to fees pool where there are treated as native tokens. | ||
| // if native transfer fails, add to credit | ||
| bool success = feesPool.withdraw(depositTo_, nativeAmount_); | ||
| bool success = feesPool.withdraw(depositToEvmxAddress_, nativeAmount_); | ||
|
|
||
| if (!success) { | ||
| _mint(depositTo_, creditAmount_); | ||
| _mint(depositToEvmxAddress_, creditAmount_); | ||
| nativeAmount_ = 0; | ||
| creditAmount_ += nativeAmount_; | ||
| } | ||
| } | ||
|
|
||
| if (data_.length > 0) | ||
| IReceiver(depositTo_).onTransfer( | ||
| IReceiver(depositToEvmxAddress_).onTransfer( | ||
| chainSlug_, | ||
| token_, | ||
| // onChainTokenAddress_, | ||
| address(0), // TODO: token_ param is not used and in future it will be removed so we can safely set it to address(0) | ||
| creditAmount_, | ||
| nativeAmount_, | ||
| data_ | ||
| ); | ||
|
|
||
| emit Deposited(chainSlug_, token_, depositTo_, creditAmount_, nativeAmount_); | ||
| emit Deposited( | ||
| chainSlug_, | ||
| onChainTokenAddress_, | ||
| depositToEvmxAddress_, | ||
| creditAmount_, | ||
| nativeAmount_ | ||
| ); | ||
| } |
There was a problem hiding this comment.
Deposit flow incorrectly uses withdraw and can double-mint on failure.
- Calling feesPool.withdraw in deposit contradicts the comment and likely sends ETH out instead of crediting the pool.
- On failure you mint creditAmount_ again, doubling credits; then you add 0 back (nativeAmount_ has been zeroed).
This should not ship. Fix the accounting and use a deposit path to feesPool; if deposit fails, mint nativeAmount_ credits (not creditAmount_) or revert.
- _mint(depositToEvmxAddress_, creditAmount_);
+ _mint(depositToEvmxAddress_, creditAmount_);
if (nativeAmount_ > 0) {
- // This is taking credit tokens and sending them to fees pool where there are treated as native tokens.
- // if native transfer fails, add to credit
- bool success = feesPool.withdraw(depositToEvmxAddress_, nativeAmount_);
- if (!success) {
- _mint(depositToEvmxAddress_, creditAmount_);
- nativeAmount_ = 0;
- creditAmount_ += nativeAmount_;
- }
+ // Convert nativeAmount_ portion into native in FeesPool.
+ bool success = feesPool.depositNative{value: 0}(depositToEvmxAddress_, nativeAmount_); // adjust to actual API
+ if (!success) {
+ // Fallback: credit the native portion instead of duplicating credits
+ _mint(depositToEvmxAddress_, nativeAmount_);
+ nativeAmount_ = 0;
+ }
}If FeesPool lacks depositNative, wire the correct method or revert on failure to keep accounting sound.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 206-249, the deposit flow
incorrectly calls feesPool.withdraw (which contradicts the intent and can send
funds out) and on failure double-mints creditAmount_ instead of handling
nativeAmount_ correctly; to fix, call the correct feesPool deposit method (e.g.,
feesPool.depositNative or the appropriate deposit function) to credit the pool
rather than withdraw, or if that method doesn't exist revert the transaction; on
a deposit failure, do not mint creditAmount_ again — either revert or mint
exactly nativeAmount_ as credits (and only after saving nativeAmount_ to a temp
variable) so you don’t double-mint, update tokenOnChainBalances and emitted
values consistently, and ensure nativeAmount_ is zeroed only after its value has
been used for minting/refund logic.
| // TODO:7: same question as in wrap function: why we do feesPool.withdraw() during unwrap ???? | ||
| function unwrap(uint256 amount_, address evmxReceiver_) external { | ||
| if (balanceOf(msg.sender) < amount_) revert InsufficientCreditsAvailable(); | ||
|
|
||
| // Burn tokens from sender | ||
| _burn(msg.sender, amount_); | ||
|
|
||
| bool success = feesPool.withdraw(receiver_, amount_); | ||
| bool success = feesPool.withdraw(evmxReceiver_, amount_); | ||
| if (!success) revert InsufficientBalance(); | ||
|
|
||
| emit CreditsUnwrapped(receiver_, amount_); | ||
| emit CreditsUnwrapped(evmxReceiver_, amount_); | ||
| } |
There was a problem hiding this comment.
unwrap is missing override specifier.
This overrides IFeesManager.unwrap but lacks override, which will not compile. Add override (and keep visibility/modifiers aligned with the interface).
-function unwrap(uint256 amount_, address evmxReceiver_) external {
+function unwrap(uint256 amount_, address evmxReceiver_) external override {📝 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.
| // TODO:7: same question as in wrap function: why we do feesPool.withdraw() during unwrap ???? | |
| function unwrap(uint256 amount_, address evmxReceiver_) external { | |
| if (balanceOf(msg.sender) < amount_) revert InsufficientCreditsAvailable(); | |
| // Burn tokens from sender | |
| _burn(msg.sender, amount_); | |
| bool success = feesPool.withdraw(receiver_, amount_); | |
| bool success = feesPool.withdraw(evmxReceiver_, amount_); | |
| if (!success) revert InsufficientBalance(); | |
| emit CreditsUnwrapped(receiver_, amount_); | |
| emit CreditsUnwrapped(evmxReceiver_, amount_); | |
| } | |
| // TODO:7: same question as in wrap function: why we do feesPool.withdraw() during unwrap ???? | |
| function unwrap(uint256 amount_, address evmxReceiver_) external override { | |
| if (balanceOf(msg.sender) < amount_) revert InsufficientCreditsAvailable(); | |
| // Burn tokens from sender | |
| _burn(msg.sender, amount_); | |
| bool success = feesPool.withdraw(evmxReceiver_, amount_); | |
| if (!success) revert InsufficientBalance(); | |
| emit CreditsUnwrapped(evmxReceiver_, amount_); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 267 to 278, the unwrap function
is implementing IFeesManager.unwrap but lacks the required override specifier;
update the function signature to include the override keyword and match the
interface's visibility and modifiers (e.g., external override and any other
modifiers from the interface) so the declaration compiles and adheres to the
interface contract.
| // function mint(address to, uint256 amount, bytes memory data_) external onlyWatcher | ||
| function mint(address to, uint256 amount, bytes memory data_) external { | ||
| _mintWithData(to, amount, data_); | ||
| } |
There was a problem hiding this comment.
Unrestricted mint is a critical vulnerability.
The onlyWatcher guard was removed. Anyone can mint credits arbitrarily. Restore access control.
-// function mint(address to, uint256 amount, bytes memory data_) external onlyWatcher
-function mint(address to, uint256 amount, bytes memory data_) external {
+function mint(address to, uint256 amount, bytes memory data_) external onlyWatcher {
_mintWithData(to, amount, data_);
}📝 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 mint(address to, uint256 amount, bytes memory data_) external onlyWatcher | |
| function mint(address to, uint256 amount, bytes memory data_) external { | |
| _mintWithData(to, amount, data_); | |
| } | |
| function mint(address to, uint256 amount, bytes memory data_) external onlyWatcher { | |
| _mintWithData(to, amount, data_); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 280 to 283, the mint function has
had the onlyWatcher access control removed, allowing anyone to mint credits;
restore access control by re-adding the onlyWatcher modifier (or an equivalent
require check that enforces the same watcher-only permission) to the mint
function signature (or inside the function), ensure the modifier is
defined/imported and applied exactly as other sensitive functions use it, and
run/update tests to confirm unauthorized callers are rejected and authorized
watcher can mint.
| address creditHolder_, | ||
| address spender_, | ||
| uint256 amount_ | ||
| ) public view override returns (bool) { | ||
| // If consumeFrom_ is not same as spender_ or spender_ is not watcher, check if it is approved | ||
| if (!_isWatcher(spender_) && consumeFrom_ != spender_) { | ||
| if (allowance(consumeFrom_, spender_) == 0) return false; | ||
| // If creditHolder_ is not same as spender_ or spender_ is not watcher, check if it is approved | ||
| if (!_isWatcher(spender_) && creditHolder_ != spender_) { | ||
| if (allowance(creditHolder_, spender_) == 0) return false; | ||
| } | ||
|
|
||
| return balanceOf(consumeFrom_) >= amount_; | ||
| return balanceOf(creditHolder_) >= amount_; | ||
| } |
There was a problem hiding this comment.
Allowance check is incorrect in isCreditSpendable.
You only check allowance != 0. This should check allowance >= amount_. Otherwise spenders can drain beyond allowance.
- if (!_isWatcher(spender_) && creditHolder_ != spender_) {
- if (allowance(creditHolder_, spender_) == 0) return false;
- }
+ if (!_isWatcher(spender_) && creditHolder_ != spender_) {
+ if (allowance(creditHolder_, spender_) < amount_) return false;
+ }📝 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.
| address creditHolder_, | |
| address spender_, | |
| uint256 amount_ | |
| ) public view override returns (bool) { | |
| // If consumeFrom_ is not same as spender_ or spender_ is not watcher, check if it is approved | |
| if (!_isWatcher(spender_) && consumeFrom_ != spender_) { | |
| if (allowance(consumeFrom_, spender_) == 0) return false; | |
| // If creditHolder_ is not same as spender_ or spender_ is not watcher, check if it is approved | |
| if (!_isWatcher(spender_) && creditHolder_ != spender_) { | |
| if (allowance(creditHolder_, spender_) == 0) return false; | |
| } | |
| return balanceOf(consumeFrom_) >= amount_; | |
| return balanceOf(creditHolder_) >= amount_; | |
| } | |
| address creditHolder_, | |
| address spender_, | |
| uint256 amount_ | |
| ) public view override returns (bool) { | |
| // If creditHolder_ is not same as spender_ or spender_ is not watcher, check if it is approved | |
| if (!_isWatcher(spender_) && creditHolder_ != spender_) { | |
| if (allowance(creditHolder_, spender_) < amount_) return false; | |
| } | |
| return balanceOf(creditHolder_) >= amount_; | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 330 to 340, the allowance check
in isCreditSpendable only tests allowance != 0 which permits spenders to
transfer more than their allowance; change the logic to compare the allowance
against amount_ (e.g. require allowance(creditHolder_, spender_) >= amount_) so
that if allowance < amount_ the function returns false before checking balance,
ensuring spenders cannot exceed their approved amount.
| export const setFeesPlugSolana = async ( | ||
| feesManagerAddress: string, | ||
| feesPlugSolanaProgramId: string | ||
| ) => { | ||
| let contractInstance = await getInstance( | ||
| Contracts.FeesManager, | ||
| feesManagerAddress | ||
| ); | ||
| contractInstance = await contractInstance.connect(getWatcherSigner()); | ||
|
|
||
| const tx = await contractInstance.setFeesPlugSolana(toBytes32FormatHexString(feesPlugSolanaProgramId)); | ||
| console.log( | ||
| `Setting FeesPlug Solana program id to ${feesPlugSolanaProgramId}, txHash: ${tx.hash}` | ||
| ); | ||
| await tx.wait(); |
There was a problem hiding this comment.
Guard env, reuse base58→bytes32 for setFeesPlugSolana().
Ensure the program ID is a bytes32 from base58 before sending on-chain.
-export const setFeesPlugSolana = async (
+export const setFeesPlugSolana = async (
feesManagerAddress: string,
feesPlugSolanaProgramId: string
) => {
let contractInstance = await getInstance(
Contracts.FeesManager,
feesManagerAddress
);
contractInstance = await contractInstance.connect(getWatcherSigner());
- const tx = await contractInstance.setFeesPlugSolana(toBytes32FormatHexString(feesPlugSolanaProgramId));
+ const tx = await contractInstance.setFeesPlugSolana(
+ feesPlugSolanaProgramId.startsWith("0x")
+ ? toBytes32FormatHexString(feesPlugSolanaProgramId)
+ : toBytes32FromBase58(feesPlugSolanaProgramId)
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/3.configureChains.ts around lines 537 to 551, guard
and normalize the Solana program id before sending it on-chain: check an env
flag (e.g. process.env.CONVERT_SOLANA_PROGRAM_ID or a new flag) or verify if the
supplied feesPlugSolanaProgramId is already a 0x-prefixed bytes32 hex (length
66); if not, convert the base58 program id to bytes32 using the existing
base58→bytes32 helper and then pass the resulting hex into
toBytes32FormatHexString before calling setFeesPlugSolana; ensure the code logs
which branch ran (converted vs already-bytes32) and only performs conversion
when necessary.
| const appGatewayAddress = process.env.APP_GATEWAY; | ||
| if (!appGatewayAddress) throw new Error("APP_GATEWAY is not set"); | ||
| const solanaSwitchboard = process.env.SWITCHBOARD_SOLANA!.slice(2); // remove 0x prefix for Buffer from conversion | ||
| if (!solanaSwitchboard) throw new Error("SWITCHBOARD_SOLANA is not set"); | ||
|
|
||
| const solanaSwitchboardBytes32 = Buffer.from(solanaSwitchboard, "hex"); | ||
| const solanaAppGatewayId = ethers.utils.hexZeroPad(appGatewayAddress, 32); | ||
|
|
||
| console.log("SolanaAppGatewayId: ", solanaAppGatewayId); | ||
| console.log( | ||
| "SolanaSwitchboardBytes32: ", | ||
| solanaSwitchboardBytes32.toString("hex") | ||
| ); | ||
|
|
||
| // On Solana there is just one switchboard, and Id indexing starts from 1 | ||
| const solanaSwitchboardId = 1; | ||
|
|
||
| appConfigs.push({ | ||
| plugConfig: { | ||
| appGatewayId: solanaAppGatewayId, | ||
| switchboard: "0x" + solanaSwitchboardBytes32.toString("hex"), | ||
| switchboardId: solanaSwitchboardId.toString(), | ||
| }, | ||
| plug: "0x" + mockForwarderSolanaOnChainAddress32Bytes.toString("hex"), | ||
| chainSlug: ChainSlug.SOLANA_DEVNET, | ||
| plug: "0x" + superTokenSolanaAddress32Bytes.toString("hex"), | ||
| chainSlug: ChainSlug.SOLANA_MAINNET, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix bytes32 serialization for Solana IDs/programs (pass 0x-prefixed 32-byte hex).
setAppGatewayConfigs will expect bytes32-like values. You serialize EVM plugs via toBytes32FormatHexString but push Solana values as raw/env strings. This should always normalize to 0x + 32-byte hex; otherwise encoding will revert or produce incorrect on-chain config.
Apply this diff to normalize Solana IDs consistently:
- const solanaAppGatewayId = ethers.utils.hexZeroPad(appGatewayAddress, 32);
+ const solanaAppGatewayId = toBytes32FormatHexString(appGatewayAddress);
appConfigs.push({
plugConfig: {
appGatewayId: solanaAppGatewayId,
- switchboardId: solanaSwitchboardId.toString(),
+ switchboardId: solanaSwitchboardId.toString(),
},
- plug: "0x" + superTokenSolanaAddress32Bytes.toString("hex"),
+ plug: "0x" + superTokenSolanaAddress32Bytes.toString("hex"),
chainSlug: ChainSlug.SOLANA_MAINNET,
});
// TODO: This is only for game testing purposes
ifAllowedAddSolanaGamePlugs(appConfigs, solanaSwitchboardId);And inside ifAllowedAddSolanaGamePlugs:
- const userVaultGameGatewayId = ethers.utils.hexZeroPad(
- process.env.USER_VAULT_GATEWAY_SOLANA!,
- 32
- );
+ const userVaultGameGatewayId = toBytes32FormatHexString(process.env.USER_VAULT_GATEWAY_SOLANA!);
const vaultPlugSolanaAddress = process.env.VAULT_PLUG_SOLANA!;
appConfigs.push({
plugConfig: {
appGatewayId: userVaultGameGatewayId,
switchboardId: solanaSwitchboardId.toString(),
},
- plug: vaultPlugSolanaAddress,
+ plug: toBytes32FormatHexString(vaultPlugSolanaAddress),
chainSlug: ChainSlug.SOLANA_MAINNET,
});
- const gameGameGatewayId = ethers.utils.hexZeroPad(
- process.env.GAME_GATEWAY_SOLANA!,
- 32
- );
+ const gameGameGatewayId = toBytes32FormatHexString(process.env.GAME_GATEWAY_SOLANA!);
const gamePlugSolanaAddress = process.env.GAME_PLUG_SOLANA!;
appConfigs.push({
plugConfig: {
appGatewayId: gameGameGatewayId,
switchboardId: solanaSwitchboardId.toString(),
},
- plug: gamePlugSolanaAddress,
+ plug: toBytes32FormatHexString(gamePlugSolanaAddress),
chainSlug: ChainSlug.SOLANA_MAINNET,
});
const feesManagerId = ethers.utils.hexZeroPad(
EVMxAddresses[Contracts.FeesManager],
32
);
const feesPlugSolanaAddress = process.env.SOLANA_FEES_PLUG_PROGRAM_ID!;
appConfigs.push({
plugConfig: {
appGatewayId: feesManagerId,
switchboardId: solanaSwitchboardId.toString(),
},
- plug: feesPlugSolanaAddress,
+ plug: toBytes32FormatHexString(feesPlugSolanaAddress),
chainSlug: ChainSlug.SOLANA_MAINNET,
});Run this to spot any remaining non-0x or non-66-char (0x + 64) plug/appGateway IDs:
Also applies to: 229-271
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 'plug:\s*["\'](?!0x)[^"\']+["\']|appGatewayId:\s*["\'](?!0x)[^"\']+["\']|plug:\s*process\.env|appGatewayId:\s*process\.env' hardhat-scripts/deploy/6.connect.tsLength of output: 267
🏁 Script executed:
#!/bin/bash
# View the target lines in the file
echo "=== Lines 181-196 ==="
sed -n '181,196p' hardhat-scripts/deploy/6.connect.ts
echo ""
echo "=== Lines 229-271 ==="
sed -n '229,271p' hardhat-scripts/deploy/6.connect.ts
echo ""
echo "=== Search for toBytes32FormatHexString function ==="
rg -n 'toBytes32FormatHexString' hardhat-scripts/deploy/6.connect.tsLength of output: 2156
Normalize Solana ID serialization to consistently use toBytes32FormatHexString for 0x-prefixed 32-byte hex.
Lines 181–196 and 229–271 mix serialization methods: some IDs use ethers.utils.hexZeroPad(), others pass raw env strings without 0x prefix. This creates encoding inconsistency when setAppGatewayConfigs processes these values on-chain, risking silent encoding errors or reverts.
All Solana plug/appGatewayId values should normalize through toBytes32FormatHexString() (already imported and used elsewhere). Apply these fixes:
Line 183: Replace ethers.utils.hexZeroPad(appGatewayAddress, 32) with toBytes32FormatHexString(appGatewayAddress)
Lines 230–233: Replace ethers.utils.hexZeroPad(process.env.USER_VAULT_GATEWAY_SOLANA!, 32) with toBytes32FormatHexString(process.env.USER_VAULT_GATEWAY_SOLANA!)
Line 236: Replace plug: vaultPlugSolanaAddress with plug: toBytes32FormatHexString(vaultPlugSolanaAddress)
Lines 243–246: Replace ethers.utils.hexZeroPad(process.env.GAME_GATEWAY_SOLANA!, 32) with toBytes32FormatHexString(process.env.GAME_GATEWAY_SOLANA!)
Line 249: Replace plug: gamePlugSolanaAddress with plug: toBytes32FormatHexString(gamePlugSolanaAddress)
Lines 256–259: Replace ethers.utils.hexZeroPad(EVMxAddresses[Contracts.FeesManager], 32) with toBytes32FormatHexString(EVMxAddresses[Contracts.FeesManager])
Line 262: Replace plug: feesPlugSolanaAddress with plug: toBytes32FormatHexString(feesPlugSolanaAddress)
🤖 Prompt for AI Agents
hardhat-scripts/deploy/6.connect.ts lines 181–196, 229–271: Solana 32-byte IDs
are inconsistently serialized (mix of ethers.utils.hexZeroPad, raw env strings
and 0x-prefixed values) which can cause encoding errors on-chain; replace all
occurrences listed in the review with the centralized helper
toBytes32FormatHexString so every appGatewayId and plug value is produced with a
0x-prefixed 32-byte hex string — specifically: at line 183 use
toBytes32FormatHexString(appGatewayAddress); at lines 230–233 use
toBytes32FormatHexString(process.env.USER_VAULT_GATEWAY_SOLANA!); at line 236
set plug to toBytes32FormatHexString(vaultPlugSolanaAddress); at lines 243–246
use toBytes32FormatHexString(process.env.GAME_GATEWAY_SOLANA!); at line 249 set
plug to toBytes32FormatHexString(gamePlugSolanaAddress); at lines 256–259 use
toBytes32FormatHexString(EVMxAddresses[Contracts.FeesManager]); and at line 262
set plug to toBytes32FormatHexString(feesPlugSolanaAddress); ensure the
toBytes32FormatHexString import is used consistently and remove the
hexZeroPad/raw usages.
| const shouldRegisterSolanGamePlugs = | ||
| process.env.IS_GAME_PLUG_REGISTRATION_ALLOWED === "true"; | ||
|
|
||
| if (shouldRegisterSolanGamePlugs) { | ||
| const userVaultGameGatewayId = ethers.utils.hexZeroPad( | ||
| process.env.USER_VAULT_GATEWAY_SOLANA!, | ||
| 32 | ||
| ); |
There was a problem hiding this comment.
Validate required env when enabling game plugs; fix typos.
If IS_GAME_PLUG_REGISTRATION_ALLOWED=true but any required env is missing, this should fail fast with a clear error. Also fix variable names.
Apply:
-const shouldRegisterSolanGamePlugs =
- process.env.IS_GAME_PLUG_REGISTRATION_ALLOWED === "true";
+const shouldRegisterSolanaGamePlugs =
+ process.env.IS_GAME_PLUG_REGISTRATION_ALLOWED === "true";
+if (shouldRegisterSolanaGamePlugs) {
+ const required = [
+ "USER_VAULT_GATEWAY_SOLANA",
+ "VAULT_PLUG_SOLANA",
+ "GAME_GATEWAY_SOLANA",
+ "GAME_PLUG_SOLANA",
+ "SOLANA_FEES_PLUG_PROGRAM_ID",
+ ] as const;
+ for (const k of required) {
+ if (!process.env[k]) throw new Error(`${k} is not set`);
+ }
+}And rename:
-const shouldRegisterSolanGamePlugs = ...
-if (shouldRegisterSolanGamePlugs) {
+const shouldRegisterSolanaGamePlugs = ...
+if (shouldRegisterSolanaGamePlugs) {
...
-const gameGameGatewayId = ...
+const gameGatewayId = ...
- appGatewayId: gameGameGatewayId,
+ appGatewayId: gameGatewayId,Also applies to: 239-243, 244-257, 258-271
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/6.connect.ts around lines 226-233 (and also apply same
changes to 239-243, 244-257, 258-271): when IS_GAME_PLUG_REGISTRATION_ALLOWED is
"true" validate all required environment variables before using them and fail
fast with clear errors (e.g. if process.env.USER_VAULT_GATEWAY_SOLANA or any
other env used in these blocks is missing, throw new Error with a descriptive
message); replace direct non-null assertions with explicit checks and hexZeroPad
only after validation; and correct any variable name typos by renaming them
consistently across all affected blocks so identifiers match their intended
meaning.
| const evmxAddresses = addresses[EVMX_CHAIN_ID] as ChainAddressesObj; | ||
| const signer: Wallet = getWatcherSigner(); | ||
|
|
||
| let deployUtils: DeployParams = { | ||
| addresses: evmxAddresses, | ||
| mode, | ||
| signer: signer, | ||
| currentChainSlug: EVMX_CHAIN_ID as ChainSlug, | ||
| }; |
There was a problem hiding this comment.
Guard against missing addresses before upgrade.
This should fail fast if EVMX addresses or AsyncDeployer are missing; otherwise .attach() or .setForwarderImplementation will throw later with poor diagnostics.
-const evmxAddresses = addresses[EVMX_CHAIN_ID] as ChainAddressesObj;
+const evmxAddresses = addresses[EVMX_CHAIN_ID] as ChainAddressesObj;
+if (!evmxAddresses) throw new Error(`No addresses for EVMX_CHAIN_ID=${EVMX_CHAIN_ID}`);
@@
-const asyncDeployer = await getInstance(
+if (!deployUtils.addresses[Contracts.AsyncDeployer]) {
+ throw new Error("AsyncDeployer address not set for EVMx");
+}
+const asyncDeployer = await getInstance(
Contracts.AsyncDeployer,
deployUtils.addresses[Contracts.AsyncDeployer]
);Also applies to: 58-66
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/UpgradeForwarderSolana.ts around lines 23-31 and
58-66, guard against missing EVMX chain addresses and missing AsyncDeployer
before using .attach() or .setForwarderImplementation: validate that
addresses[EVMX_CHAIN_ID] is defined and non-empty and throw a clear Error if
not; ensure getWatcherSigner() returned a signer (throw if falsy); ensure the
DeployParams includes a valid AsyncDeployer (or throw if AsyncDeployer is not
present) before proceeding; add these fast-fail checks at the start of the
function so you fail early with descriptive error messages rather than allowing
.attach()/.setForwarderImplementation to throw later with poor diagnostics.
| // This must run each time we deploy new AG with Socket protocol (tmp change just for game testing) | ||
| const ADDRESS_TO_WHITELIST = "0x45d810BA4071429278958bBbd3AA68bFCDA37b00"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Temporary hardcoded address should use environment variable.
The "tmp change just for game testing" comment indicates this is temporary test data. Hardcoding addresses in deployment scripts creates risk if committed to main branches.
Consider using an environment variable instead:
-// This must run each time we deploy new AG with Socket protocol (tmp change just for game testing)
-const ADDRESS_TO_WHITELIST = "0x45d810BA4071429278958bBbd3AA68bFCDA37b00";
+const ADDRESS_TO_WHITELIST = process.env.WHITELIST_ADDRESS || "0x45d810BA4071429278958bBbd3AA68bFCDA37b00";Or verify this temporary change should be committed.
🏁 Script executed:
#!/bin/bash
# Verify the file exists and examine the code context
fd -t f "WhitelistFeesReceiver.ts" --exec cat {}Length of output: 1393
🏁 Script executed:
#!/bin/bash
# Look for environment variable usage patterns in deployment scripts
fd -t f -e ts -e js "deploy" --exec grep -l "process.env" {} \; | head -20Length of output: 75
🏁 Script executed:
#!/bin/bash
# Check if similar hardcoded addresses exist in other deployment scripts
rg -n "0x[a-fA-F0-9]{40}" --type ts hardhat-scripts/ -C2 | head -40Length of output: 2602
🏁 Script executed:
#!/bin/bash
# Search for .env or environment configuration files
fd -t f "\.env" .Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about deployment practices
fd -t f -e md "deploy\|config" . | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine how 1.deploy.ts uses process.env for address configuration
rg -n "process.env" hardhat-scripts/deploy/1.deploy.ts -A2 -B2Length of output: 144
This should use an environment variable to prevent hardcoded test addresses in deployment scripts.
The codebase already establishes the pattern—other deployment scripts use process.env for configuration (e.g., process.env.EVMX_RPC in 1.deploy.ts). Since dotenv is already imported here, using process.env.WHITELIST_ADDRESS aligns with existing conventions and prevents the temporary address from being committed accidentally. Hardcoding means every address change requires code edits instead of env var changes.
The suggested diff is correct: use an environment variable with a fallback to the current hardcoded value.
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/WhitelistFeesReceiver.ts around lines 10-11, the
whitelisted address is hardcoded which risks committing test addresses; replace
the hardcoded constant with a lookup from the environment such as reading
process.env.WHITELIST_ADDRESS and falling back to the current literal if not
set, ensure dotenv is imported/initialized at the top of the file so env vars
are loaded, and update any usage to reference the new variable instead of the
literal.
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 (1)
hardhat-scripts/deploy/1.deploy.ts (1)
131-142: Remove the unnecessary empty string length check.The
feePool?.length == 0condition is defensive bloat with no basis. The feePools mapping stores only non-empty Ethereum addresses; it never contains empty strings. The only waygetFeePool()returns falsy is whenmodeisn't in feePools (returnsundefined). Theundefinedcheck alone suffices:if (feePool === undefined) { // deploy new FeesPool }This should be simplified to remove the length check and the confusing optional chaining.
♻️ Duplicate comments (3)
foundry.toml (1)
2-17: This duplicates the existing review comment about upgrading to Solidity 0.8.30 and reconsidering the global optimizer_runs=1 setting.hardhat-scripts/deploy/1.deploy.ts (1)
157-175: This duplicates the existing review comment about replacing the invalid "0x0" placeholder withethers.constants.AddressZeroand asserting nonzero before passing to FeesManager.EventTopics.md (1)
67-88: This duplicates the existing critical review comment about outdated/incorrect event topic hashes that don't match current contract event definitions. The topic hashes must be recomputed using proper Keccak256 implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
EventTopics.md(4 hunks)deployments/stage_addresses.json(1 hunks)deployments/stage_verification.json(3 hunks)foundry.toml(1 hunks)hardhat-scripts/config/config.ts(4 hunks)hardhat-scripts/constants/feeConstants.ts(1 hunks)hardhat-scripts/deploy/1.deploy.ts(4 hunks)hardhat-scripts/utils/overrides.ts(1 hunks)setupInfraContracts.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- setupInfraContracts.sh
🔇 Additional comments (11)
hardhat-scripts/utils/overrides.ts (1)
55-57: Justify the 2.5x gasLimit increase for BASE.This increases the BASE chain gasLimit from 2M to 5M without explanation. This could unnecessarily increase transaction costs if the limit is higher than required.
Verify this change is necessary by checking:
- Recent transaction failures or gas limit issues on BASE
- Actual gas consumption of transactions that motivated this increase
- Whether this is a temporary workaround or permanent configuration
If this is based on actual gas usage data, add a comment documenting the rationale.
hardhat-scripts/config/config.ts (2)
35-35: Verify downstream impact of LOCAL evmChainId change.The evmChainId changed from 7625382 to 14323. This breaks compatibility with any existing LOCAL deployments or tooling that references the old chain ID.
Ensure that:
- All deployment scripts, contract addresses, and indexers are updated for the new chain ID
- Any off-chain services or frontends are aware of this change
- The old chain ID deployments are intentionally being deprecated
44-64: Confirm reduced DEV chain coverage is intentional.DEV mode chains are now limited to ARBITRUM_SEPOLIA and OPTIMISM_SEPOLIA only, with all mainnet chains commented out. This significantly reduces test coverage compared to typical DEV setups.
Confirm this reduction is intentional for the "Surge gameplay iteration stage" and that broader chain testing will be restored before production deployment.
hardhat-scripts/deploy/1.deploy.ts (1)
51-54: LGTM: Constant renamed to reflect production usage.The rename from
mockForwarderSolanaOnChainAddress32BytestosuperTokenSolanaAddress32Bytesaligns with moving from test to mainnet Solana integration.hardhat-scripts/constants/feeConstants.ts (1)
345-349: LGTM: Fee pool addresses updated for LOCAL and STAGE environments.Address changes align with the broader ForwarderSolana integration and deployment infrastructure updates across the PR.
EventTopics.md (4)
77-77: New Solana integration events require indexer/listener updates.Two new events and one modified event introduce Solana-specific fields:
FeesPlugSolanaSet(feesPlugSolanaProgramId: bytes32)- new eventSusdcTokenSetparameter renamed fromsusdcTokentosusdcContractIdEnsure monitoring and indexing infrastructure is updated to capture these events.
Also applies to: 84-84
149-154: LGTM: ForwarderSolana section added with standard Initialized event.The new ForwarderSolana contract follows the standard initialization pattern.
343-346: New Solana debugging events in WritePrecompile.Three new events expose Solana instruction processing details:
SolanaDecodedInstruction(instruction: tuple)SolanaFunctionArgsPacked(functionArgsPacked: bytes)SolanaInstructionInput(payload: bytes)These appear to be debugging/diagnostic events for Solana instruction handling. Verify whether these should remain in production or be feature-gated for development environments only.
75-75: No in-codebase event listeners found, but breaking change requires external verification.The script found no TypeScript/JavaScript event listeners or hardcoded topic hash references in the repository. The Deposited event signature change is real and documented, but verification must occur outside this codebase—off-chain listeners, indexers, and blockchain explorers tracking this event must be updated separately before deployment to handle the new signature and topic hash.
deployments/stage_verification.json (1)
2-239: Verify all updated addresses match actual stage deployments.This file contains extensive address updates across multiple networks (8453, 14323, 42161). These changes introduce new contracts (FeesPool, ForwarderSolana) and update existing contract addresses. Ensure all addresses are verified against actual on-chain deployments before merging.
deployments/stage_addresses.json (1)
2-56: Consistency verified—no action required.Verification confirms all addresses in stage_addresses.json match stage_verification.json:
- ForwarderSolana and ForwarderSolanaImpl entries for chain 14323 present and consistent (0xa75db4a2c602d567b0414B6893cFfcD4C903a1dF confirmed in both)
- FeesPool address matches (0x4f3322415BDF6c07107186d0015aEdCA9F4f1Ca9)
- Removed chains (10, 84532, 421614) correctly absent from both files
- Chain 14323 properly registered in verification file with all contract references
The changes are intentional and consistent across both deployment metadata files.
hardhat-scripts/config/config.ts
Outdated
| evmChainId: 14323, // dummy stage | ||
| // evmChainId: 12921, |
There was a problem hiding this comment.
STAGE evmChainId is set to dummy value.
The evmChainId is configured with a "dummy stage" value (14323) while the actual value (12921) is commented out. This incomplete configuration could cause issues if STAGE mode is deployed.
Uncomment and use the correct evmChainId before deploying to STAGE:
- evmChainId: 14323, // dummy stage
- // evmChainId: 12921,
+ // evmChainId: 14323, // dummy stage
+ evmChainId: 12921,📝 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.
| evmChainId: 14323, // dummy stage | |
| // evmChainId: 12921, | |
| // evmChainId: 14323, // dummy stage | |
| evmChainId: 12921, |
🤖 Prompt for AI Agents
In hardhat-scripts/config/config.ts around lines 102 to 103 the STAGE evmChainId
is set to a placeholder value 14323 while the actual STAGE chain id 12921 is
commented out; replace the dummy value by uncommenting and using 12921 (remove
or update the commented line accordingly) so the STAGE configuration uses the
correct evmChainId before deployment.
hardhat-scripts/config/config.ts
Outdated
| // watcher: "0xb62505feacC486e809392c65614Ce4d7b051923b", | ||
| watcher: "0x09ce244745923f250A1E53e95fEd51E9De01a129", | ||
| transmitter: "0x138e9840861C983DC0BB9b3e941FB7C0e9Ade320", | ||
| socketOwner: "0x3339Cf48f1F9cf31b6F8c2664d144c7444eBBB18", | ||
| // socketOwner: "0x3339Cf48f1F9cf31b6F8c2664d144c7444eBBB18", | ||
| socketOwner: "0x09ce244745923f250A1E53e95fEd51E9De01a129", |
There was a problem hiding this comment.
Security risk: watcher and socketOwner consolidated to single address.
Both the watcher and socketOwner roles are now controlled by the same address (0x09ce244745923f250A1E53e95fEd51E9De01a129). This creates a single point of failure and centralization risk for critical protocol functions.
This should be split to separate addresses for defense in depth. If this consolidation is temporary for testing:
- Document the security implications
- Ensure proper key management controls are in place
- Restore separate addresses before production deployment
🤖 Prompt for AI Agents
In hardhat-scripts/config/config.ts around lines 105 to 109 the watcher and
socketOwner are set to the same address which centralizes control and creates a
single point of failure; change socketOwner to a distinct address (or restore
the previously commented value) and update the file comment to document if this
consolidation is temporary, include a TODO to restore separate keys before
production, and ensure you reference separate private keys / key-management
entries for watcher and socketOwner in your environment/config so each role is
controlled by different credentials.
|
|
||
| const feePool = getFeePool(); | ||
| if (feePool?.length == 0) { | ||
| console.log("XXX feePool:", feePool); |
There was a problem hiding this comment.
Remove debug log before production deployment.
The "XXX" prefix indicates this is a temporary debug statement that should be removed.
- console.log("XXX feePool:", feePool);📝 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("XXX feePool:", feePool); |
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/1.deploy.ts around line 129, there is a temporary
debug console.log("XXX feePool:", feePool) that should be removed before
production deployment; delete this debug statement (or replace it with a proper
logger call if persistent info is required) so no "XXX" debug output remains in
the deployed code.
…tests tfor Borsh encoder
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)
script/helpers/DepositCredit.s.sol (1)
9-9: Fix stale comment - script name mismatch.The comment references
DepositCreditAndNative.s.solbut the actual file isDepositCredit.s.sol. This will confuse anyone trying to run the script.Apply this diff:
-// source .env && forge script script/helpers/DepositCreditAndNative.s.sol --broadcast --skip-simulation +// source .env && forge script script/helpers/DepositCredit.s.sol --broadcast --skip-simulationhardhat-scripts/config/config.ts (1)
128-139: Stage config must match the newly deployed watcher/socketOwner
deployments/stage_addresses.json now exposesWatcherat0xB5c99FDb167500fcb775E870d82B9aABbD15b0E0, but Stage mode here still exports the legacy0xb625.... Any Stage script that readswatcher/socketOwnerfrom config will sign or authorize with the wrong account, so multi-call updates and owner checks will fail. This should be updated to the fresh addresses emitted in stage_addresses.json (and any other changed Stage admin keys) before the next run.- addresses: { - watcher: "0xb62505feacC486e809392c65614Ce4d7b051923b", - transmitter: "0x138e9840861C983DC0BB9b3e941FB7C0e9Ade320", - socketOwner: "0x3339Cf48f1F9cf31b6F8c2664d144c7444eBBB18", - }, + addresses: { + watcher: "0xB5c99FDb167500fcb775E870d82B9aABbD15b0E0", + transmitter: "0x138e9840861C983DC0BB9b3e941FB7C0e9Ade320", + socketOwner: "0x3339Cf48f1F9cf31b6F8c2664d144c7444eBBB18", // update if Stage owner rotated + },hardhat-scripts/deploy/8.setupEnv.ts (1)
28-53: Restoring ARBITRUM_ envs to the correct chain slug is required*
Switching tolatestAddresses[ChainSlug.ARBITRUM]hard-points these env vars at mainnet even whenmodeis DEV or STAGE (both rely on Arbitrum Sepolia). That misconfiguration will make downstream scripts connect to the wrong contracts. This should branch onmode, keeping Sepolia for non-prod and mainnet only for PROD.-import { EVMX_CHAIN_ID, mode } from "../config/config"; +import { EVMX_CHAIN_ID, mode } from "../config/config"; +import { DeploymentMode } from "../../src"; … -const arbAddresses = latestAddresses[ChainSlug.ARBITRUM] as ChainAddressesObj; +const arbChainSlug = + mode === DeploymentMode.PROD ? ChainSlug.ARBITRUM : ChainSlug.ARBITRUM_SEPOLIA; +const arbAddresses = latestAddresses[arbChainSlug] as ChainAddressesObj;
♻️ Duplicate comments (5)
EventTopics.md (2)
67-87: Event topic verification still required.The past review identified that many event topics don't match the actual contract signatures. The new events added in this PR (FeesPlugSolanaSet, updated Deposited and SusdcTokenSet signatures) must also be verified using proper Keccak256 computation against the actual contract event definitions.
343-345: Verify new Solana event topics.These three new Solana-related events (SolanaDecodedInstruction, SolanaFunctionArgsPacked, SolanaInstructionInput) need topic hash verification against the actual contract signatures in WritePrecompile.sol.
contracts/evmx/watcher/precompiles/WritePrecompile.sol (2)
70-73: Remove debug events before production.These debug events leak payload data and increase gas costs. The TODO confirms they're temporary debugging tools.
100-102: View modifier cannot be restored due to event emissions.The
viewmodifier was removed because_createSolanaDigestParams(line 315) now emits events, making the entire call chain non-view. This creates a breaking change for integrations expecting a read-only call. Either make the debug events conditional and restoreview, or document this breaking change.hardhat-scripts/deploy/6.connect.ts (1)
181-285: Normalize every Solana appGatewayId/plug to 0x-prefixed bytes32
This block still mixeshexZeroPadand raw env strings, sosetAppGatewayConfigsreceives values that are not guaranteed to be bytes32 (most of these env vars are base58 Solana program IDs). Encoding will revert or store garbage on-chain. This should consistently run throughtoBytes32FormatHexString, as flagged earlier.- const solanaAppGatewayId = ethers.utils.hexZeroPad(appGatewayAddress, 32); + const solanaAppGatewayId = toBytes32FormatHexString(appGatewayAddress); … - const userVaultGameGatewayId = ethers.utils.hexZeroPad( - process.env.USER_VAULT_GATEWAY_SOLANA!, - 32 - ); + const userVaultGameGatewayId = toBytes32FormatHexString( + process.env.USER_VAULT_GATEWAY_SOLANA! + ); … - plug: vaultPlugSolanaAddress, + plug: toBytes32FormatHexString(vaultPlugSolanaAddress), … - const gameGameGatewayId = ethers.utils.hexZeroPad( - process.env.GAME_GATEWAY_SOLANA!, - 32 - ); + const gameGatewayId = toBytes32FormatHexString( + process.env.GAME_GATEWAY_SOLANA! + ); … - plug: gamePlugSolanaAddress, + plug: toBytes32FormatHexString(gamePlugSolanaAddress), … - const feesManagerId = ethers.utils.hexZeroPad( - EVMxAddresses[Contracts.FeesManager], - 32 - ); + const feesManagerId = toBytes32FormatHexString( + EVMxAddresses[Contracts.FeesManager] + ); … - plug: feesPlugSolanaAddress, + plug: toBytes32FormatHexString(feesPlugSolanaAddress), … - plug: susdcSolanaAddress, + plug: toBytes32FormatHexString(susdcSolanaAddress),Also rename
gameGameGatewayIdaccordingly. This aligns with the previous review requirement and guarantees deterministic 32-byte serialization.
🧹 Nitpick comments (1)
script/helpers/DepositCredit.s.sol (1)
22-26: Consider removing redundant debug logs.Lines 22 and 26 log static strings (
"testUSDCContract.mint","testUSDCContract.approve") that provide minimal debugging value. The comprehensive logging at lines 30-35 already captures the essential state. If these were added temporarily for debugging, they can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(2 hunks)Errors.md(1 hunks)EventTopics.md(3 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(5 hunks)deployments/stage_addresses.json(1 hunks)deployments/stage_verification.json(3 hunks)foundry.toml(1 hunks)hardhat-scripts/config/config.ts(1 hunks)hardhat-scripts/deploy/6.connect.ts(4 hunks)hardhat-scripts/deploy/8.setupEnv.ts(2 hunks)package.json(2 hunks)script/helpers/DepositCredit.s.sol(1 hunks)setupInfraContracts.sh(2 hunks)test/BorshDecoderTest.t.sol(40 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- package.json
- foundry.toml
🧰 Additional context used
🧬 Code graph analysis (2)
hardhat-scripts/deploy/8.setupEnv.ts (1)
src/types.ts (1)
ChainAddressesObj(18-33)
hardhat-scripts/deploy/6.connect.ts (4)
hardhat-scripts/deploy/1.deploy.ts (1)
superTokenSolanaAddress32Bytes(51-54)hardhat-scripts/constants/types.ts (2)
AppGatewayConfig(14-21)DeploymentAddresses(3-5)hardhat-scripts/utils/address.ts (1)
getAddresses(8-27)hardhat-scripts/config/config.ts (2)
mode(10-12)EVMX_CHAIN_ID(212-212)
🔇 Additional comments (2)
Errors.md (1)
20-25: Error signatures verified and correctly documented.Both
InvalidSolanaChainSlug()andAddressResolverNotSet()are properly defined in ForwarderSolana.sol and actively used in the contract logic. The error signatures match the documentation exactly, so the selector hashes (0xe37803ab and 0x6d55276d) will be correctly computed by Solidity at runtime. No issues found.script/helpers/DepositCredit.s.sol (1)
11-37: LGTM - functional logic is sound.The script correctly mints test USDC, approves the FeesPlug contract, and deposits credit. The configuration changes align with the broader Base Sepolia migration.
| // TODO: this is temporary, must be injected from function arguments | ||
| // bytes32 of Solana Socket address : 9vFEQ5e3xf4eo17WttfqmXmnqN3gUicrhFGppmmNwyqV | ||
| bytes32 hardcodedSocket = 0x84815e8ca2f6dad7e12902c39a51bc72e13c48139b4fb10025d94e7abea2969c; | ||
| // bytes32 of Solana transmitter address : pFCBP4bhqdSsrWUVTgqhPsLrfEdChBK17vgFM7TxjxQ - local testing | ||
| // bytes32 transmitterSolana = 0x0c1a5886fe1093df9fc438c296f9f7275b7718b6bc0e156d8d336c58f083996d; | ||
| // bytes32 of Solana transmitter address : 8cg7rkZn8M5WKZohbvgYTp4nMaYAyGt4qZeftewGeHU2 - stage testing | ||
| bytes32 transmitterSolana = 0x71255e30f1493eea868d431b2aa93f65f439afe739a6cf123dd5cf1096564a97; | ||
| return | ||
| DigestParams( | ||
| // watcherPrecompileConfig__.sockets(params_.payloadHeader.getChainSlug()), // TODO: this does not work, for some reason it returns 0x000.... address | ||
| hardcodedSocket, | ||
| toBytes32Format(transmitter_), | ||
| // toBytes32Format(transmitter_), | ||
| transmitterSolana, |
There was a problem hiding this comment.
Remove hardcoded Solana addresses.
Hardcoded socket (line 338) and transmitter addresses (lines 340-342) make this contract environment-specific and non-configurable. The TODOs and multiple commented transmitter addresses for different environments confirm this is temporary scaffolding. These values should be injected via constructor/initialization or retrieved from a configuration contract.
This should be refactored to accept transmitter from the caller or configuration:
function _createSolanaDigestParams(
PayloadParams memory payloadParams_,
Transaction memory transaction_,
address appGateway_,
- address /*transmitter_*/,
+ address transmitter_,
bytes32 prevBatchDigestHash_,
uint256 deadline_,
uint256 gasLimit_,
uint256 value_
) internal returns (DigestParams memory) {
// ... decoding logic ...
- // TODO: this is temporary, must be injected from function arguments
- bytes32 hardcodedSocket = 0x84815e8ca2f6dad7e12902c39a51bc72e13c48139b4fb10025d94e7abea2969c;
- // bytes32 of Solana transmitter address : pFCBP4bhqdSsrWUVTgqhPsLrfEdChBK17vgFM7TxjxQ - local testing
- // bytes32 transmitterSolana = 0x0c1a5886fe1093df9fc438c296f9f7275b7718b6bc0e156d8d336c58f083996d;
- // bytes32 of Solana transmitter address : 8cg7rkZn8M5WKZohbvgYTp4nMaYAyGt4qZeftewGeHU2 - stage testing
- bytes32 transmitterSolana = 0x71255e30f1493eea868d431b2aa93f65f439afe739a6cf123dd5cf1096564a97;
+ bytes32 socket = configurations__().sockets(transaction_.chainSlug);
+ bytes32 transmitterSolana = toBytes32Format(transmitter_);
+
return
DigestParams(
- hardcodedSocket,
- // toBytes32Format(transmitter_),
- transmitterSolana,
+ socket,
+ transmitterSolana,
// ... rest of paramsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
contracts/evmx/watcher/precompiles/WritePrecompile.sol lines 336-348: the
function currently returns DigestParams with hardcoded Solana socket and
transmitter addresses which must be removed; instead add injectable
storage/config values and use them here—introduce either
constructor/initializer-settable state variables or read from an external
configuration contract (or accept the socket/transmitter as function
parameters), validate non-zero values, update the DigestParams assembly to use
these state/config/argument values, and ensure any call sites are updated to
provide the transmitter/socket or the contract is initialized before use.
| // bytes32 transmitterSolana = 0x0c1a5886fe1093df9fc438c296f9f7275b7718b6bc0e156d8d336c58f083996d; | ||
| // bytes32 of Solana transmitter address : 8cg7rkZn8M5WKZohbvgYTp4nMaYAyGt4qZeftewGeHU2 - stage testing | ||
| bytes32 transmitterSolana = 0x71255e30f1493eea868d431b2aa93f65f439afe739a6cf123dd5cf1096564a97; | ||
| return | ||
| DigestParams( | ||
| // watcherPrecompileConfig__.sockets(params_.payloadHeader.getChainSlug()), // TODO: this does not work, for some reason it returns 0x000.... address | ||
| hardcodedSocket, | ||
| toBytes32Format(transmitter_), | ||
| // toBytes32Format(transmitter_), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented code and investigate configuration issue.
Commented addresses (lines 340-341, 347) and the comment at line 346 suggesting configurations__().sockets() returns zero for Solana chains indicate a deeper configuration system issue. Remove the commented code and ensure Solana chain slugs are properly registered in the configuration contract.
🤖 Prompt for AI Agents
In contracts/evmx/watcher/precompiles/WritePrecompile.sol around lines 340 to
347, remove the dead commented address lines and the commented call to
sockets/getter, then fix the configuration registration so
watcherPrecompileConfig__.sockets(chainSlug) returns the correct socket for
Solana: ensure the Solana chain slug is registered in the config contract
(correct key/format), confirm the sockets mapping and its getter are
public/accessible, update the deployment/migration to write the socket entry for
the Solana slug, and add a unit/integration check that
watcherPrecompileConfig__.sockets(solanaSlug) != address(0) to prevent
regressions.
| vm.createSelectFork(vm.envString("BASE_SEPOLIA_RPC")); | ||
|
|
||
| uint256 privateKey = vm.envUint("PRIVATE_KEY"); | ||
| vm.startBroadcast(privateKey); | ||
| FeesPlug feesPlug = FeesPlug(payable(vm.envAddress("ARBITRUM_FEES_PLUG"))); | ||
| FeesPlug feesPlug = FeesPlug(payable(vm.envAddress("BASE_FEES_PLUG"))); | ||
| address appGateway = vm.envAddress("APP_GATEWAY"); | ||
| TestUSDC testUSDCContract = TestUSDC(vm.envAddress("ARBITRUM_TEST_USDC")); | ||
| console.log("appGateway", appGateway); | ||
| TestUSDC testUSDCContract = TestUSDC(vm.envAddress("BASE_TEST_USDC")); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify BASE environment variables are documented.
The migration from ARBITRUM to BASE environment variables requires BASE_SEPOLIA_RPC, BASE_FEES_PLUG, and BASE_TEST_USDC to be properly set and documented. Ensure these contracts are deployed on Base Sepolia and the environment variables are added to documentation/README.
🏁 Script executed:
#!/bin/bash
# Verify if BASE environment variables are documented in .env files or README
# Search for BASE_SEPOLIA_RPC, BASE_FEES_PLUG, BASE_TEST_USDC in documentation
rg -g '*.md' -g '*.env*' 'BASE_SEPOLIA_RPC|BASE_FEES_PLUG|BASE_TEST_USDC'Length of output: 55
🏁 Script executed:
#!/bin/bash
# Search for all references to FEES_PLUG and TEST_USDC in the codebase
rg -i 'FEES_PLUG|TEST_USDC' --type-list
rg 'ARBITRUM_FEES_PLUG|ARBITRUM_TEST_USDC|BASE_FEES_PLUG|BASE_TEST_USDC'
# Also check .env.sample full content
echo "=== .env.sample content ==="
cat .env.sample 2>/dev/null || echo ".env.sample not found"Length of output: 2536
Add missing BASE environment variables to .env.sample and update setup script.
The code references BASE_FEES_PLUG and BASE_TEST_USDC (lines 17, 20) but these are not documented in .env.sample—only the ARBITRUM equivalents are. The hardhat setup script (hardhat-scripts/deploy/8.setupEnv.ts) also only populates ARBITRUM vars, leaving BASE network unhandled.
Add BASE_FEES_PLUG and BASE_TEST_USDC to .env.sample and extend the setup script to populate these for Base Sepolia, mirroring the ARBITRUM pattern.
🤖 Prompt for AI Agents
In script/helpers/DepositCredit.s.sol around lines 13 to 20, the script
references BASE_FEES_PLUG and BASE_TEST_USDC but the .env.sample and the hardhat
setup script only include ARBITRUM variants; add BASE_FEES_PLUG and
BASE_TEST_USDC entries to .env.sample (with placeholder values and comments) and
update hardhat-scripts/deploy/8.setupEnv.ts to populate these BASE_SEPOLIA
variables (mirror the ARBITRUM logic: read/generate addresses/values and write
BASE_FEES_PLUG and BASE_TEST_USDC into the .env file or env output) so the Base
Sepolia run has the required env vars.
| function testDecodeRugPoolReturnData() public pure { | ||
| GenericSchema memory dataSchema; | ||
| dataSchema.valuesTypeNames = new string[](5); | ||
| dataSchema.valuesTypeNames[0] = "u32"; | ||
| dataSchema.valuesTypeNames[1] = "u64"; | ||
| dataSchema.valuesTypeNames[2] = "u64"; | ||
| dataSchema.valuesTypeNames[3] = "u64"; | ||
| dataSchema.valuesTypeNames[4] = "u64"; | ||
|
|
||
| bytes | ||
| memory triggerData = hex"8196980067f6495700000000fcfbbbd20000000064273fae00000000f7a303ff00000000"; | ||
|
|
||
| bytes[] memory decodedData = BorshDecoder.decodeGenericSchema(dataSchema, triggerData); | ||
|
|
||
| uint32 chainSlug = abi.decode(decodedData[0], (uint32)); | ||
| uint64 lpTokensPulled = abi.decode(decodedData[1], (uint64)); | ||
| uint64 lpBalance = abi.decode(decodedData[2], (uint64)); | ||
| uint64 susdcInPool = abi.decode(decodedData[3], (uint64)); | ||
| uint64 warcoinInPool = abi.decode(decodedData[4], (uint64)); | ||
|
|
||
| console.log("chainSlug: ", chainSlug); | ||
| console.log("lpTokensPulled: ", lpTokensPulled); | ||
| console.log("lpBalance: ", lpBalance); | ||
| console.log("susdcInPool: ", susdcInPool); | ||
| console.log("warcoinInPool: ", warcoinInPool); | ||
|
|
||
| // // console.log("decoded data: "); | ||
| // uint64 decodedSusdcPoolSupply = abi.decode(decodedData[0], (uint64)); | ||
| // assertEq(decodedSusdcPoolSupply, 8927759588); | ||
| // // console.log(decodedSusdcPoolSupply); | ||
| // uint64 decodedWarCoinPoolSupply = abi.decode(decodedData[1], (uint64)); | ||
| // assertEq(decodedWarCoinPoolSupply, 10014755499); | ||
| // // console.log(decodedWarCoinPoolSupply); | ||
| // uint64 decodedDepositorLpTokensAdded = abi.decode(decodedData[2], (uint64)); | ||
| // assertEq(decodedDepositorLpTokensAdded, 518418); | ||
| // // console.log(decodedDepositorLpTokensAdded); | ||
| } |
There was a problem hiding this comment.
Add assertions and remove commented code.
This test only logs decoded values without verifying correctness. Add assertions to validate the decoded fields match expected values. The commented block (lines 802-811) references different field names and should be either removed or uncommented if needed.
Apply this diff to add assertions:
console.log("chainSlug: ", chainSlug);
console.log("lpTokensPulled: ", lpTokensPulled);
console.log("lpBalance: ", lpBalance);
console.log("susdcInPool: ", susdcInPool);
console.log("warcoinInPool: ", warcoinInPool);
+
+ // Add assertions to verify decoded values
+ assertEq(chainSlug, 10000002);
+ assertEq(lpTokensPulled, 1467893607);
+ assertEq(lpBalance, 59192828);
+ assertEq(susdcInPool, 2934730596);
+ assertEq(warcoinInPool, 4277088247);
- // // console.log("decoded data: ");
- // uint64 decodedSusdcPoolSupply = abi.decode(decodedData[0], (uint64));
- // assertEq(decodedSusdcPoolSupply, 8927759588);
- // // console.log(decodedSusdcPoolSupply);
- // uint64 decodedWarCoinPoolSupply = abi.decode(decodedData[1], (uint64));
- // assertEq(decodedWarCoinPoolSupply, 10014755499);
- // // console.log(decodedWarCoinPoolSupply);
- // uint64 decodedDepositorLpTokensAdded = abi.decode(decodedData[2], (uint64));
- // assertEq(decodedDepositorLpTokensAdded, 518418);
- // // console.log(decodedDepositorLpTokensAdded);
}🤖 Prompt for AI Agents
In test/BorshDecoderTest.t.sol around lines 776 to 812, the test only logs
decoded values and leaves an unrelated commented block; add assertions that
verify each decoded field equals the expected literal values
(assertEq(chainSlug, <expected_u32>), assertEq(lpTokensPulled, <expected_u64>),
assertEq(lpBalance, <expected_u64>), assertEq(susdcInPool, <expected_u64>),
assertEq(warcoinInPool, <expected_u64>)) and remove the obsolete commented code
block so the test fails on mismatch rather than silently logging.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores