- 
                Notifications
    You must be signed in to change notification settings 
- Fork 75
feat(Linea): Add CCTP V2 support to Linea #910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>
Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me 👍
* fix(ZkSync_SpokePool): Add __gap This contract gets extended by the Lens_SpokePool which doesn't add any storage but we should add it in case a future variable gets added to the Lens_SpokePool * Update ZkSync_SpokePool.json
Very messy, but allows us to switch between V1 and V2 at construction time without contract changes
…USDT0` via `OFT` messaging protocol (#902) * first draft of OFTTransportAdapter Signed-off-by: Ihor Farion <ihor@umaproject.org> * update yarn.lock file Signed-off-by: Ihor Farion <ihor@umaproject.org> * Revert "update yarn.lock file" This reverts commit 4c216ea. Signed-off-by: Ihor Farion <ihor@umaproject.org> * add yarn.lock compatible with master Signed-off-by: Ihor Farion <ihor@umaproject.org> * polish OFTTransportAdapter, add OFT support to Arbitrum_Adapter on L1, and Arbitrum_SpokePool on L2 Signed-off-by: Ihor Farion <ihor@umaproject.org> * polish + fix missing approval Signed-off-by: Ihor Farion <ihor@umaproject.org> * add context for dstEid Signed-off-by: Ihor Farion <ihor@umaproject.org> * address most of the PR comments about contracts Signed-off-by: Ihor Farion <ihor@umaproject.org> * update deploy scripts, add tests for OFT messaging, polish contracts Signed-off-by: Ihor Farion <ihor@umaproject.org> * cleanup comments and extraneous log file Signed-off-by: Ihor Farion <ihor@umaproject.org> * revert package.json prepublish change Signed-off-by: Ihor Farion <ihor@umaproject.org> * generalize oft adapter to support multiple tokens. Introduce OFTAddressBook to support that. Update deploy / test scripts to reflect new functionality Signed-off-by: Ihor Farion <ihor@umaproject.org> * add __gap to ArbitrumSpokePool, update stale comments on OFTTransportAdapter, update layouts of ArbitrumSpokePool and AlephZeroSpokePool Signed-off-by: Ihor Farion <ihor@umaproject.org> * update some comments; adjust fee cap naming Signed-off-by: Ihor Farion <ihor@umaproject.org> * address PR comments Signed-off-by: Ihor Farion <ihor@umaproject.org> * address PR comments Signed-off-by: Ihor Farion <ihor@umaproject.org> * fix deploy script, remove incorrect values from consts Signed-off-by: Ihor Farion <ihor@umaproject.org> * improve comment Signed-off-by: Ihor Farion <ihor@umaproject.org> * add oftFeeCap as a param to arbitrum adapter construction Signed-off-by: Ihor Farion <ihor@umaproject.org> * move OFT functionality to SpokePool for easy further integration and removing boilerplate code Signed-off-by: Ihor Farion <ihor@umaproject.org> * remove layerzero from foundry remappings Signed-off-by: Ihor Farion <ihor@umaproject.org> * address licensing comment; add permalink to LZ OFT code on github Signed-off-by: Ihor Farion <ihor@umaproject.org> * fix a couple of comment typos Signed-off-by: Ihor Farion <ihor@umaproject.org> --------- Signed-off-by: Ihor Farion <ihor@umaproject.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| // implement feeRecipient but it has a fallback function so to be extra safe, we check the return value | ||
| // of feeRecipient() as well. | ||
| (bool success, bytes memory feeRecipient) = address(cctpTokenMessenger).staticcall( | ||
| abi.encodeWithSignature("feeRecipient()") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I want to highlight to OZ.
We are doing this to avoid adding an extra constructor argument and, thus, create a larger diff for all the contracts that inherit from this CCTP contract. However, the downside is that this could be broken in the future if circle removes this function.
The base branch was changed.
* feat(Linea): Add CCTP V2 support to Linea (#910) * fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 - The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first). - The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32. - Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor. * Revert "fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1" This reverts commit cd36e36. * fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 (#924) - The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first). - The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32. - Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor. * improve(CCTPAdapter): Add comment about not casting tokenMinter to V1/V2 (#928) * improve(cctpAdapter): Fix misleading comments (#929) * improve(cctpAdapter): typographical errors (#930) * improve(CCTPAdapter): Remove comment (#931) * fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 - The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first). - The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32. - Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor. * Remove comment about cctp domain IDs * fix(CCTPAdapter): Typographical errors (#935)
This PR adds a new CCTPV2 Adapter that we add to the Linea_Adapter and Linea_SpokePool.
CCTP V2 has a different interface than V1 but it has a similar on-chain flow--so we can re-use some of the code.