Required gas, events, reentrancy, go linter #66
Conversation
minghinmatthewlam
requested review from
michaelkaplan13,
cam-schultz,
geoff-vball and
bernard-avalabs
as code owners
| // Need to figure out appropriate values. | ||
| uint256 public constant SEND_TOKENS_REQUIRED_GAS = 300_000; | ||
| /// @notice Required gas limit for sending tokens to another chain. | ||
| uint256 public constant SEND_TOKENS_REQUIRED_GAS = 80_000; |
There was a problem hiding this comment.
Given that token source contracts are intended to be compatible with any destination contracts that implement the required interface, I think we should have the gas limit for messages sent from source -> destination be provided as a parameter for each call to send. A destination contracts can have arbitrarily complex logic, so we shouldn't try to fit one gas limit to all of them.
It could make sense to have some minimum or default that we don't allow the passed value to be less than, but I think we'll need to allow for callers to specify higher values if needed.
There was a problem hiding this comment.
added requiredGasLimit to SendTokensInput since destination token bridges also need to pass in case for multihop.
There was a problem hiding this comment.
The required gas limit for message from destination token bridge -> source is still a constant in TeleporterTokenDestination though
There was a problem hiding this comment.
I think the same logic applies to messages to the source, since the source can be composed with an arbitrary ERC20, which can also execute arbitrary logic.
There was a problem hiding this comment.
Good point, maybe we should have the value set in the constructor of TeleporterTokenDestination? Its value should be more or less constant for a given source contract, but it could change from one source to another as Cam called out.
| @@ -34,8 +34,8 @@ abstract contract TeleporterTokenDestination is | |||
| /// @notice The ERC20 token this contract uses to pay for Teleporter fees. | |||
| address public immutable feeTokenAddress; | |||
|
|
|||
| /// @notice Required gas limit for sending tokens to another chain. | |||
| uint256 public constant SEND_TOKENS_REQUIRED_GAS = 220_000; | |||
| /// @notice Required gas limit for sending tokens back to the source blockchain. | |||
There was a problem hiding this comment.
A few asks for this comment:
- Can you update this comment to describe the multihop case? Right now, it reads more like a point to point transfer.
- Can you clarify that this figure accounts for the fixed gas relaying the transfer to a third chain? Also can you mention what blockchain this gas is spent on?
There was a problem hiding this comment.
updated comment to clarify
| * @param secondaryFee amount of tokens to pay for Teleporter fee if a multihop is needed. | ||
| * @param allowedRelayerAddresses addresses of relayers allowed to send the message | ||
| * @param secondaryFee amount of tokens to pay for Teleporter fee if a multihop is needed | ||
| * @param requiredGasLimit gas limit requirement for sending to a destination token bridge |
There was a problem hiding this comment.
Can you explain that this is required because a source/destination bridge contracts are able to call arbitrary code?
minghinmatthewlam
dismissed stale reviews from michaelkaplan13, geoff-vball, and cam-schultz
via
c26d823
minghinmatthewlam
requested review from
geoff-vball,
michaelkaplan13 and
cam-schultz
Why this should be merged
Fixes #23 #55 #36
How this works
nonReentrantfor external sendsWithdrawTokenseventSendTokensInputtoSendTokenseventforge test --gas-reportand testing with e2e testsHow this was tested
forgeHow is this documented