feat: tron batch payments contracts and tests#1724
Conversation
Greptile SummaryThis PR introduces Tron-specific batch payment contracts and tests to the
Confidence Score: 4/5Safe to merge with minor caveats in the new ERC20BatchPayments contract. The contract logic is sound: tokens are pulled from the payer before proxy calls, Solidity 0.8 overflow protection prevents integer wrapping, and the batch-fee formula matches the test helper exactly. The two items worth attention are both non-critical: the zero-address sentinel collision in the multi-token aggregation loop is a confusing-but-safe degenerate input, and the public visibility of packages/smart-contracts/tron/contracts/ERC20BatchPayments.sol — specifically the unique-token aggregation loop and the visibility of Important Files Changed
Sequence DiagramsequenceDiagram
participant Payer
participant Batch as ERC20BatchPayments
participant Proxy as ERC20FeeProxy
participant ERC20
participant Payee
participant Fee as FeeAddress
Payer->>Batch: batchERC20PaymentsWithReference(token, payees, amounts, refs, fees, feeAddr)
Batch->>Batch: validate array lengths
Batch->>Batch: sum total amountAndFee
Batch->>ERC20: check allowance and balance
Batch->>ERC20: safeTransferFrom(payer, batch, totalAmountAndFee)
Batch->>ERC20: approve(proxy, MAX) if needed
loop for each payment
Batch->>Proxy: transferFromWithReferenceAndFee(token, payee, amount, ref, fee, feeAddr)
Proxy->>ERC20: transferFrom(batch, payee, amount)
Proxy->>ERC20: transferFrom(batch, feeAddr, fee)
end
Batch-->>Payer: success or full revert
Reviews (4): Last reviewed commit: "fix: remove return statement" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const expectRevertOrNoBalanceChange = async (fn, getBalances) => { | ||
| const before = await getBalances(); | ||
| try { | ||
| await fn(); | ||
| } catch (_error) {} | ||
| await waitForConfirmation(2000); | ||
| const after = await getBalances(); | ||
| const unchanged = before.every((value, index) => value === after[index]); | ||
| return { unchanged }; | ||
| }; |
There was a problem hiding this comment.
expectRevertOrNoBalanceChange catches errors without asserting that a revert actually occurred. If the transaction unexpectedly succeeds and the balance check happens to track the wrong account, a funds leak would be silently accepted. The expectNonOwnerReverts helper in the same file correctly asserts threw === true; applying the same pattern here would make failure detection explicit.
There was a problem hiding this comment.
On Tron, when a revert occurs the transaction call does not fail.
See this transaction for example.
1 participant
Description of the changes
Adds Tron-specific batch payment contracts and test infrastructure to the
smart-contractspackage.This PR introduces the existing EVM
BatchPaymentcontract on Tron, and introducesERC20BatchPayments,a version for better performance and stripped down of unused methods on TronTwo Tron-targeted test suites are added:
ERC20BatchPayments.test.jscoversERC20BatchPayments, including happy-path single-token and multi-token payments, zero-amount payments, zero-fee payments,BadTRC20token handling, and error cases such as insufficient funds, missing allowance, and mismatched input arrays.BatchPayments.test.jscovers the mainBatchPaymentscontract on Tron, adding batch fee computation and verification, dynamic batch fee updates, admin access control for proxy and fee setters, and a check thatbatchEthPaymentsWithReferencereverts when noEthFeeProxyis configured.A shared
helpers.jsmodule provides utilities used by both test files, including token deployment, approval helpers, balance diffing, batch fee computation, and revert/no-balance-change assertion helpers.The root
package.jsonworkspace configuration is updated to prevent hoisting of@openzeppelindependencies for thesmart-contractspackage, ensuring the Tron build resolves its own copy of those contracts.Contracts Scope
BatchPayments.sol
Same contract as the one on EVM.
We don't have the EthFeeProxy on Tron, so the associated address is defined as the zero address. Calls to the associated method fail as expected.
ERC20BatchPayment.sol
This is the same contract as above, with existing
BatchPayments without:EthFeeProxymethodsSecurity Consideration
Currently, both contracts provide methods for managing the underlying proxies. For better security, we could hardcode them at deployment time, since they are unlikely to change anytime soon, especially since we don't own a multisig on Tron.
The
ERC20BatchPaymentcontract also benefits from not exposing unused methods, thereby reducing the attack surface. Given that there is no batch fee, it makes it easier to completely remove the notion of contract ownerGas Consideration
ERC20BatchPaymenthas a lower gas footprint because it does not include batch-fee-related logic.Initially, I created a third version in which the
feeAmountsandfeeRecipientswere removed from the interface and were always set to zero within the contract. This didn't reduce the gas cost compared to a call where we would pass the zero values directly.