Conversation
|
Original PR for reference: https://github.com/centrifuge/protocol-internal/pull/74 |
wischli
left a comment
There was a problem hiding this comment.
Thanks for re-opening and good job on putting this all together. I have a few questions before merging though.
|
|
||
| /// @title TokenBridge | ||
| /// @notice Wrapper contract for cross-chain token transfers compatible with Glacis Airlift | ||
| contract TokenBridge is Auth, ITokenBridge { |
There was a problem hiding this comment.
Q for all of us: Should this contract implement Recoverable to protect against accidently sent tokens?
There was a problem hiding this comment.
Looks like it's never used as a target for receiving tokens. I would say not (?)
| } | ||
|
|
||
| /// @inheritdoc ITrustedContractUpdate | ||
| function trustedCall(PoolId poolId, ShareClassId scId, bytes memory payload) external auth { |
There was a problem hiding this comment.
Nit: We can use calldata payload. I discovered this as part of optimizing ARM size code. We still have several code paths in the repo using memory despite the interface declaration of calldata.
| function trustedCall(PoolId poolId, ShareClassId scId, bytes memory payload) external auth { | |
| function trustedCall(PoolId poolId, ShareClassId scId, bytes calldata payload) external auth { |
|
|
||
| /// @inheritdoc ITokenBridge | ||
| function send(address token, uint256 amount, bytes32 receiver, uint256 destinationChainId, address refundAddress) | ||
| public |
There was a problem hiding this comment.
Should this really be public or instead external?
| poolId, | ||
| scId, | ||
| receiver, | ||
| uint128(amount), |
There was a problem hiding this comment.
CR: Should use safe method from Mathlib to improve debugging in case this edge case occurs
| uint128(amount), | |
| amount.toUint128(), |
| SafeTransferLib.safeApprove(token, address(spoke), type(uint256).max); | ||
| } | ||
|
|
||
| (PoolId poolId, ShareClassId scId) = spoke.shareTokenDetails(token); |
There was a problem hiding this comment.
Nit: We should move this up before the actual transfer because it is theoretically fallible such that we can save costs in case this check fails.
| error InvalidChainId(); | ||
| error InvalidRelayer(); |
There was a problem hiding this comment.
Wanted to flag these are not yet used in the codebase
| error InvalidToken(); | ||
| error UnknownTrustedCall(); | ||
| error ShareTokenDoesNotExist(); | ||
| error FailedToTransferToRelayer(); |
| bridge.send(shareToken1, DEFAULT_AMOUNT, receiver.toBytes32(), invalidChainId, user); | ||
| } | ||
|
|
||
| function testSendInvalidToken() public { |
There was a problem hiding this comment.
Great tests! Could you add one for revoking a chain mapping by setting centrifugeId to 0 and then verifying send() reverts? And maybe also one for catching uint128(amount) overflow.
| protocolGuardian.fileTokenBridgeRelayer(makeAddr("relayer")); | ||
| } | ||
|
|
||
| function testfileCentrifugeIdSuccess() public { |
There was a problem hiding this comment.
Nit: Incorrect camelcase
| function testfileCentrifugeIdSuccess() public { | |
| function testFileCentrifugeIdSuccess() public { |
| protocolGuardian.fileTokenBridgeCentrifugeId(evmChainId, CENTRIFUGE_ID); | ||
| } | ||
|
|
||
| function testfileCentrifugeIdRevertWhenNotSafe() public { |
There was a problem hiding this comment.
| function testfileCentrifugeIdRevertWhenNotSafe() public { | |
| function testFileCentrifugeIdRevertWhenNotSafe() public { |
| function _relyAdapters(FullReport memory report, address ward) internal { | ||
| if (address(report.layerZeroAdapter) != address(0)) report.layerZeroAdapter.rely(ward); | ||
| if (address(report.wormholeAdapter) != address(0)) report.wormholeAdapter.rely(ward); | ||
| if (address(report.axelarAdapter) != address(0)) report.axelarAdapter.rely(ward); | ||
| if (address(report.chainlinkAdapter) != address(0)) report.chainlinkAdapter.rely(ward); | ||
| } |
There was a problem hiding this comment.
Nice idea to reduce bytecode!
| /// @inheritdoc ITokenBridge | ||
| function file(bytes32 what, uint256 evmChainId, uint16 centrifugeId) external auth { | ||
| if (what == "centrifugeId") chainIdToCentrifugeId[evmChainId] = centrifugeId; | ||
| else revert FileUnrecognizedParam(); | ||
| emit File(what, evmChainId, centrifugeId); | ||
| } |
There was a problem hiding this comment.
From my experience, file() methods for things other than dependencies can be somewhat messy in the end and difficult to handle. Could we use here a "normal" name like setDestination(evmChainId, centrifugeId) or something similar?
|
|
||
| /// @notice Send a token from chain A to chain B after approving this contract with the token | ||
| /// @dev This function transfers tokens from the caller and initiates a cross-chain transfer | ||
| /// @dev These methods match the expected interface from Glacis Airlift for cross-chain token transfers |
There was a problem hiding this comment.
Maybe then we should move send(..) to its own interface like IGlacisAirliftTransfer or so? to reflect that and ensure this method signature remains immutable.
Do you have a reference for where this method is defined?
|
|
||
| SafeTransferLib.safeTransferFrom(token, msg.sender, address(this), amount); | ||
| if (IERC20(token).allowance(address(this), address(spoke)) == 0) { | ||
| SafeTransferLib.safeApprove(token, address(spoke), type(uint256).max); |
There was a problem hiding this comment.
Yes, I also think the allowance is never checked. I think approval is something the user of TokenBridge should do before call to TokenBridge for leave TokenBridge to handle the transfer funds.
| uint128(amount), | ||
| limits.extraGasLimit, | ||
| limits.remoteExtraGasLimit, | ||
| relayer != address(0) ? relayer : refundAddress // Transfer remaining ETH to relayer if set |
There was a problem hiding this comment.
Why not always to the refundAddress? No fully understand the need of the relayer, could you extend?
|
|
||
| /// @title TokenBridge | ||
| /// @notice Wrapper contract for cross-chain token transfers compatible with Glacis Airlift | ||
| contract TokenBridge is Auth, ITokenBridge { |
There was a problem hiding this comment.
Looks like it's never used as a target for receiving tokens. I would say not (?)
| } | ||
|
|
||
| /// forge-config: default.isolate = true | ||
| function testSendWithRelayerSuccess() public { |
There was a problem hiding this comment.
IIUC, the relayer does not imply any new interaction with the system regarding the normal testSendSuccess(). I think the good usage of the relayer is already covered by UTs, could it be?
Product requirements
https://www.notion.so/LiFi-bridging-contract-2b62eac24e178072a5a3ed17ebe770e3?source=copy_link
Design notes
TODOs