Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
wischli
left a comment
wischli
left a comment
There was a problem hiding this comment.
Just a bunch of post-approval questions, minor nits and test additions. Great job!
|
|
||
| /// @inheritdoc ILayerZeroReceiver | ||
| function nextNonce(uint32, bytes32) external pure override returns (uint64 nonce) { | ||
| return 0; |
There was a problem hiding this comment.
Just to be sure: Based on the ILayerZero comment (ref), we return 0 here to not enforce nonce ordering, right?
| uint16 internal constant TYPE_3 = 3; | ||
| uint8 internal constant WORKER_ID = 1; | ||
| uint8 internal constant OPTION_TYPE_LZRECEIVE = 1; |
There was a problem hiding this comment.
Nit: Not a fan of storage values outside the header area
There was a problem hiding this comment.
Personally think this is a case of making an exception since it works better in this specific case. These aren't really storage variables, just configuration vars that only apply to the options. For that reason to me it seems more logical to have it grouped together.
There was a problem hiding this comment.
I understand what you want is name those "random" values. It's true it looks a bit different. Maybe we can just add variables to the method below if only used there.
There was a problem hiding this comment.
Since this is already audited, would not change this now. But yes could have been variables in the function too.
| uint8 internal constant WORKER_ID = 1; | ||
| uint8 internal constant OPTION_TYPE_LZRECEIVE = 1; |
There was a problem hiding this comment.
Since these values were opaque to me, I looked into the LayerZero-v2 ExecutorOptions to confirm these values for the most recent TYPE_3 = 3: https://github.com/LayerZero-Labs/LayerZero-v2/blob/200cda254120375f40ed0a7e89931afb897b8891/packages/layerzero-v2/evm/messagelib/contracts/libs/ExecutorOptions.sol#L10-L12
Co-authored-by: William Freudenberger <w.freude@icloud.com>
Co-authored-by: William Freudenberger <w.freude@icloud.com>
Co-authored-by: William Freudenberger <w.freude@icloud.com>
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
Looks good! Pretty similar scheme! Just some few comment, mostly NITs
| endpoint.setDelegate(delegate); | ||
| emit SetDelegate(delegate); |
There was a problem hiding this comment.
NIT, maybe we can call to setDelegate() directly
There was a problem hiding this comment.
This doesn't work, similar as https://github.com/centrifuge/protocol-v3/blob/main/src/common/Gateway.sol#L58, deployer is not auth
| using CastLib for *; | ||
| using MathLib for *; | ||
|
|
||
| IMessageHandler public immutable entrypoint; |
There was a problem hiding this comment.
Maybe we want to set this as mutable in order to avoid migrating this in v3.1
There was a problem hiding this comment.
All adapters have this now as immutable so would keep as is for now. Something to reconsider for v3.1 upgrade yeah
| /// @inheritdoc IAdapter | ||
| function estimate(uint16 centrifugeId, bytes calldata payload, uint256 gasLimit) external view returns (uint256) { | ||
| LayerZeroDestination memory destination = destinations[centrifugeId]; | ||
| MessagingFee memory fee = endpoint.quote(_params(destination, payload, gasLimit), address(this)); |
There was a problem hiding this comment.
Could the estimation fail because address(this) is not payable?
Maybe we should pass the refund address to this method too, to correctly estimate everything. Refunding will add some cost
There was a problem hiding this comment.
The last param here is not the refund address, but the sender. See https://github.com/centrifuge/protocol-v3/blob/layerzero-adapter/src/adapters/interfaces/ILayerZeroAdapter.sol#L43
| uint16 internal constant TYPE_3 = 3; | ||
| uint8 internal constant WORKER_ID = 1; | ||
| uint8 internal constant OPTION_TYPE_LZRECEIVE = 1; |
There was a problem hiding this comment.
I understand what you want is name those "random" values. It's true it looks a bit different. Maybe we can just add variables to the method below if only used there.
| } | ||
|
|
||
| function quote(MessagingParams calldata, address) external pure returns (MessagingFee memory) { | ||
| // TODO: decode gas limit from params.options |
There was a problem hiding this comment.
Yeah I will remove this TODO. It is a bit complex, and not really much value. More valuable to add the integration tests that our auditors wrote to the codebase (https://gist.github.com/gvladika/ad74ef148840879b0b60fba96278a50e)
|
Coverage after merging layerzero-adapter into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
As a note, the configuration is missing in |
tests: improve AdaptersDeploymentInputTest
3 participants
No description provided.