UniversalGateway PC + Vault Inclusion #13
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds Push Chain outbound gateway (UniversalGatewayPC) with supporting interfaces, errors, mocks, and extensive test coverage, plus a new “universal tx execution” path on the legacy UniversalGateway.
- Introduces UniversalGatewayPC with withdraw and withdraw+execute flows, fee calculation via UniversalCore, and event emission.
- Extends UniversalGateway with executeUniversalTx, approval handling, and replay protection.
- Adds comprehensive tests and mocks for PRC20, UniversalCore, ERC20 approval variants, targets, and reentrancy scenarios.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/evm-gateway/src/UniversalGatewayPC.sol | New gateway for Push Chain outbound flows; fee handling, burning, events |
| contracts/evm-gateway/src/interfaces/IUniversalGatewayPC.sol | Interface for UniversalGatewayPC, including event and admin/user APIs |
| contracts/evm-gateway/src/libraries/Errors.sol | Adds new specific errors used by PC gateway and universal execution path |
| contracts/evm-gateway/src/interfaces/IUniversalCore.sol | Minimal interface for UniversalCore (UEM, gas token, gas price) |
| contracts/evm-gateway/src/interfaces/IPRC20.sol | Minimal PRC20 interface required by gateway logic |
| contracts/evm-gateway/src/UniversalGateway.sol | Adds executeUniversalTx path, approval reset/safe approve helpers, and replay guard |
| contracts/evm-gateway/test/gateway/8_GatewayPC.t.sol | Comprehensive tests for UniversalGatewayPC withdraw flows, edge cases, and reentrancy |
| contracts/evm-gateway/test/gateway/7_GatewayExecuteUniversalTx.t.sol | Tests for executeUniversalTx: native/ERC20 paths, approvals, and failures |
| contracts/evm-gateway/test/mocks/MockPRC20.sol | Accurate PRC20 mock used by gateway tests |
| contracts/evm-gateway/test/mocks/MockUniversalCoreReal.sol | Core mock with gas price/token config and autoswap placeholders |
| contracts/evm-gateway/test/mocks/MockReentrantContract.sol | Reentrancy test helper calling gateway functions |
| contracts/evm-gateway/test/mocks/MockTarget.sol | Target mock receiving ETH or tokens via transferFrom |
| contracts/evm-gateway/test/mocks/MockRevertingTarget.sol | Target mock with revert/gas-heavy behaviors |
| contracts/evm-gateway/test/mocks/MockUSDTToken.sol | USDT-like approve-zero-first behavior |
| contracts/evm-gateway/test/mocks/MockTokenApprovalVariants.sol | Configurable token approve behavior for negative-path tests |
| contracts/evm-gateway/.env.sample | Removed sample env file (non-code) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| msg.sender, chainId, token, target, amount, gasToken, gasFee, gasLimitUsed, payload, protocolFee, revertInstruction | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
execute() fn is missing
If a user only wants to execute an outbound payload without withdrawal, then execute fn needs to be there
| /// @param target target contract address to execute call | ||
| /// @param amount amount of token to send along | ||
| /// @param payload calldata to be executed on target | ||
| function executeUniversalTx( |
There was a problem hiding this comment.
it only supports arbitrary payload execution, but not the onCall/onExecute feature, which is crucial for any universal app development
There was a problem hiding this comment.
TBD internally.
|
Second Set of Changes Made
|
| whenNotPaused | ||
| onlyRole(FUND_MANAGER_ROLE) | ||
| { | ||
| _enforceSupportedToken(token); |
There was a problem hiding this comment.
why do we want to enforce supported token restriction here?
and what's the diff between sweep and withdraw in this pc vault?
isn't this supposed to be a protocol fees only contract, then why not just two functions
- withdraw for erc20
- withdraw for native pc
There was a problem hiding this comment.
- _enforceSupportedToken acts an additional layer of security to ensure fees collected are supported PRC20 tokens themselves
- withdraw vs sweep:
- with withdraw(), admin takes fees out by legit PRC20 tokens.
- with sweep() , it acts as a security mechanism to remove/transfer any unwanted tokens that are sent to this contract.
- Do you think we need withdraw for native PC function? Just to be sure, which chain's outbound flow will charge fees in native PC token?
|
|
||
| /// @inheritdoc IUniversalGateway | ||
| function withdrawFunds(address recipient, address token, uint256 amount) | ||
| function revertUniversalTxToken(address token, uint256 amount, RevertInstructions calldata revertInstruction) |
There was a problem hiding this comment.
we can also apply replay protection in revert functions
similar to withdraw by taking txId
There was a problem hiding this comment.
works - but just to understand: every revert tx will also be a universal TX with txID?? I was not expecting txID for revert specific txs but wanted to double check this on this.
| /// @inheritdoc IUniversalGateway | ||
| function revertWithdrawFunds(address token, uint256 amount, RevertInstructions calldata revertInstruction) | ||
| function revertUniversalTx(uint256 amount, RevertInstructions calldata revertInstruction) |
There was a problem hiding this comment.
same replay protection comment here
There was a problem hiding this comment.
works - but just to understand: every revert tx will also be a universal TX with txID?? I was not expecting txID for revert specific txs but wanted to double check this on this.
| /// @inheritdoc IUniversalGateway | ||
| function revertWithdrawFunds(address token, uint256 amount, RevertInstructions calldata revertInstruction) | ||
| function revertUniversalTx(uint256 amount, RevertInstructions calldata revertInstruction) |
There was a problem hiding this comment.
even though I supported having a revert native token in gateway for a proper record :)
there are two things:
- there is no withdraw native tokens fn, TSS directly transfers it. We should make it standardised, either have both withdraw and revert for native or both should be handled by tss only without a gateway interaction
- having a fn for native withdraw and revert has benefit of proper record and all, but it unnecessarily opens up a route for re-entrancy (even tho, I don't see any issue now)
There was a problem hiding this comment.
do you still consider THIS a issue, after I responded on slack on the reason behind this? Are you suggesting we have a withdrawNative() function in gateway?
List of Important Changes in this PR
UniversalGatewayPC has now been added - a dedicated gateway for outbound flows from Push Chain.
1. New Vault Contract for ERC20 Custody
Separation of Concerns: Introduced dedicated Vault.sol contract for ERC20 token custody
Key Features:
Three main operations:
withdraw(): Simple ERC20 transfers
withdrawAndExecute(): Transfers tokens to Gateway, then triggers execution
revertWithdraw(): Refund path for failed operations
UniversalGateway Refactor: Vault Integration
Function Changes
Removed: withdrawFunds() (replaced by Vault delegation)
Added:
Token Flow Changes
_handleTokenDeposit(): Now transfers tokens directly to VAULT instead of address(this)
ERC20 custody responsibility fully delegated to Vault contract
ExecutePayloadd
Test cases and interfaces have been updated accordingly.