-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add create2 factory router #1
Conversation
* @param _salt The salt used for deploying the contract with CREATE2 | ||
* @param _bytecode The bytecode of the contract to deploy | ||
*/ | ||
function deployContract( |
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.
One thing we probably didn't specify is that ideally this router includes remote router overrides like callRemoteWithOverrides
of InterchainAccountRouter, to allow callers to reuse the existing router address on an existing chain, but not require the owner of the router to populate the routers mapping (since folks can deploy hyperlane themselves).
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.
since folks can deploy hyperlane themselves
So that's what the Overrides
is for!
Taking into account the other comment, we would need to offer to override the hook here as well and call directly to mailbox.dispatch
instead _Router_dispatch
which uses _mustHaveRemoteRouter
.
* @param _messageBody The message body to be dispatched. | ||
* @param _hookMetadata The hook metadata to override with for the hook set by the owner | ||
*/ | ||
function quoteGasPayment( |
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 would be important to offer to override the hook itself as well (if the caller wants to pay a different relayer, or use a different ISM that needs a post dispatch hook (like rollup bridges)
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 we offer to override the hook here then we should do the same for deployContract
, right? and I won't be able to user re Router._Router_quoteDispatch
because it uses _mustHaveRemoteRouter(_destinationDomain);
Something like:
function quoteDispatch(
uint32 _destinationDomain,
bytes32 _router,
bytes memory _messageBody,
bytes memory _hookMetadata,
address _hook
) internal view returns (uint256) {
return
mailbox.quoteDispatch(
_destinationDomain,
_router,
_messageBody,
_hookMetadata,
IPostDispatchHook(_hook)
);
}
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.
Indeed
Hey @nambrot I made the requested changes, let me know what you think please!! |
Sorry for the delay here, lgtm to merge! |
TODO
How to use
section