-
Notifications
You must be signed in to change notification settings - Fork 6
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
added hyperlane adapter #17
base: main
Are you sure you want to change the base?
Conversation
* @title HyperlaneReceiverAdapter implementation. | ||
* @notice `IBridgeReceiverAdapter` implementation that uses Hyperlane as the bridge. | ||
*/ | ||
contract HyperlaneReceiverAdapter is |
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.
Can you rename this to indicate that this only supports v2
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.
address srcSender | ||
) = abi.decode(_body, (MessageLib.Message[], bytes32, uint256, address)); | ||
|
||
if (IMessageDispatcher(adapter) != senderAdapters[srcChainId]) { |
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.
IMO this should compare to the origin argument from the handle, no? Otherwise an origin chain sender from a different chain can falsely claim a different chain (even if it has to be permissioned as an adapter)
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.
} | ||
if (executedMessages[msgId]) { | ||
revert MessageIdAlreadyExecuted(msgId); | ||
} else { |
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 else is technically superflous, i like having all the revert conditions up front, and then do the state changes after
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.
import { IMessageExecutor } from "./interfaces/IMessageExecutor.sol"; | ||
import "./libraries/MessageLib.sol"; | ||
|
||
contract HyperlaneSenderAdapter is ISingleMessageDispatcher, IBatchedMessageDispatcher, Ownable { |
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.
Ditto about v2
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.
bytes calldata /* data*/ | ||
) external view returns (uint256) { | ||
uint32 dstDomainId = _getDestinationDomain(toChainId); | ||
// destination gasAmount is hardcoded to 500k similar to Wormhole implementation |
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.
Why not making this a configuration option in the constructor (and still default to 500k)
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.
MessageLib.Message[] memory _messages = new MessageLib.Message[](1); | ||
_messages[0] = MessageLib.Message({ to: _to, data: _data }); | ||
|
||
bytes memory payload = abi.encode(_messages, msgId, block.chainid, msg.sender); |
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.
We generally recommend having a library that does the encoding and decoding in one place;
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.
igp.payForGas{ value: msg.value }( | ||
hyperlaneMsgId, | ||
dstDomainId, | ||
500000, |
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.
Move to constant/storage
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.
@@ -0,0 +1,109 @@ | |||
// SPDX-License-Identifier: GPL-3.0 |
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.
Why is this file here?
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.
I am using a modified ExecutorAware contract, which allows me to add and remove multiple trusted executors as opposed to the poolTogether implementation which has just one executor that can only be initialized in the constructor, although now I have moved it to its own abstract folder
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.
Is that necessary? It seems useful to be compliant with the default implementation? (its why we are doing this PR in the first place no?)
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.
I have removed it
@@ -0,0 +1,24 @@ | |||
// SPDX-License-Identifier: GPL-3.0 |
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.
All these files are already on main, no? You should import them from their canonical place
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.
I have removed the unnecessary interfaces in the adapter apart from IBatchedMessageDispatcher and ISingleMessageDispatcher, because the ones I'm using for the hyperlane adapter have their dispatchMessage and dispatchMessageBatch functions marked as payable to faciliate the hyperlane dispatch function whereas the ones defined by poolTogether don't have their functions marked as payable
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.
@asselstine Would be curious to hear how to handle this case? Should we modify the existing interface to make it payable?
|
||
/** | ||
* @title HyperlaneReceiverAdapter implementation. | ||
* @notice `IBridgeReceiverAdapter` implementation that uses Hyperlane as the bridge. | ||
*/ | ||
contract HyperlaneReceiverAdapter is | ||
contract HyperlaneReceiverAdapterV2 is |
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.
updated to V2
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.
Should this be v3?
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.
I have updated the adapter to use hyperlane V3 and added some unit tests as well
) = abi.decode(_body, (MessageLib.Message[], bytes32, uint256, address)); | ||
) = EncodeDecodeUtil.decode(_body); | ||
|
||
if (_origin != srcChainId) { |
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.
comparing the origin and sender chain
if (_messages.length < 1) { | ||
revert Errors.NoMessagesSent(srcChainId); | ||
} | ||
|
||
if (executedMessages[msgId]) { | ||
revert MessageIdAlreadyExecuted(msgId); | ||
} else { |
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.
did all my revert conditions up front and did state changes after
|
||
contract HyperlaneSenderAdapter is ISingleMessageDispatcher, IBatchedMessageDispatcher, Ownable { | ||
contract HyperlaneSenderAdapterV2 is ISingleMessageDispatcher, IBatchedMessageDispatcher, Ownable { |
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.
updated to V2
|
||
// See https://docs.hyperlane.xyz/docs/build-with-hyperlane/guides/paying-for-interchain-gas | ||
// Set gasAmount to the default (500,000) if _gasAmount is 0 | ||
gasAmount = (_gasAmount == 0) ? 500000 : _gasAmount; |
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.
made gasAmount a configurable option in the constructor, it it isn't set (set to zero), it defaults to 500k
* @title EncodeDecodeUtil | ||
* @notice Library to encode and decode payloads. | ||
*/ | ||
library EncodeDecodeUtil { |
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.
created a separate library that does the encoding and decoding
@@ -112,7 +121,7 @@ contract HyperlaneSenderAdapter is ISingleMessageDispatcher, IBatchedMessageDisp | |||
MessageLib.Message[] memory _messages = new MessageLib.Message[](1); | |||
_messages[0] = MessageLib.Message({ to: _to, data: _data }); | |||
|
|||
bytes memory payload = abi.encode(_messages, msgId, block.chainid, msg.sender); | |||
bytes memory payload = EncodeDecodeUtil.encode(_messages, msgId, block.chainid, msg.sender); |
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.
Used the library I created for encoding/decoding to encode the messages with extra relevant data to generate a payload
//address(this) | ||
msg.sender | ||
) | ||
igp.payForGas{ value: msg.value }(hyperlaneMsgId, dstDomainId, gasAmount, msg.sender) |
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.
Moved gas amount to storage as gas amount is now a state variable set in the constructor
Hey- just seeing this. It's great to see another adapter! |
hey @asselstine is this good to merge? cc our CTO @nambrot |
That's good to know, this project was a very good learning process for me as it's very impressive, and I got to learn a lot of from contributing to it. |
No description provided.