feat(cross-chain-oracle): Enable GovernorHub to change child messenger on child network#3688
feat(cross-chain-oracle): Enable GovernorHub to change child messenger on child network#3688chrismaree merged 18 commits intomasterfrom
Conversation
…r on child network
| emit ExecutedGovernanceTransaction(to, inputData); | ||
| // There is a special case if `to` is this contract. If this contract is the target, then we assume that the | ||
| // cross-chain caller is attempting to change the child messenger. | ||
| if (to == address(this)) { |
There was a problem hiding this comment.
hhmm. ideally, we'd like a separate method that does this messenger upgrade. problem is though that our inbound calls from L2->L1 always go via the processMessagerFromParent call.
I don't think we want to overload this function like this. in my mind this is a bit of a code smell. it might be better to add a function on this contract called setChildMessenger, like you did on the OracleSpoke. Then, set the access control on that function to be called only by this contract. We'd then craft a normal Governance tx, sent over the bridge like any other one and it calls back into the GovernorSpoke by setting the to to be the address of this contract. Setting the messenger would be done by calling the OracleSpoke's SetMessengerSpoke via a normal governance call (using the function you added already).
what this means is we don't overload the function like this, the GovernorSpoke does not need to have an extra event, extra branching logic or need to store the Finder. We do need to send two governance transactions at once (one for each contract's spoke setting) but I think that's fine as it makes this part of the implementation was easier.
wdyt?
There was a problem hiding this comment.
I've updated this PR to use this implementation. now, we have one entry point setChildMessenger that sets the messengers in both contexts, which is intended to be called from the processMessageFromParent function.
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
nicholaspai
left a comment
There was a problem hiding this comment.
Looks great left some nits
|
|
||
| require(_executeCall(to, inputData), "execute call failed"); | ||
| emit ExecutedGovernanceTransaction(to, inputData); | ||
| emit ExecutedGovernanceTransaction(to, data); |
There was a problem hiding this comment.
Since "to" is a part of "data" I think you should either emit "to" and "inputData" (the data you call "to" with) or just data. I prefer leaving this the way it is
| * to the processMessageFromParent function which intern calls back into this contract. | ||
| * @param newChildMessenger Address of the child messenger contract to set for this contract and OracleSpoke. | ||
| */ | ||
| function setChildMessenger(address newChildMessenger) public onlyThisContract { |
There was a problem hiding this comment.
Nit: should this be reentrant guarded?
There was a problem hiding this comment.
the method reverts if this has a reentrant guard on it :)
Signed-off-by: chrismaree <christopher.maree@gmail.com>
| let owner; | ||
| let messenger; | ||
|
|
||
| let governor; |
There was a problem hiding this comment.
small nit to make tests easier to read.
Signed-off-by: chrismaree <christopher.maree@gmail.com>
| modifier onlyGovernorSpoke() { | ||
| require( | ||
| msg.sender == finder.getImplementationAddress(OracleInterfaces.GovernorSpoke), | ||
| "Caller must be governor spoke" | ||
| ); |
There was a problem hiding this comment.
I may be totally missing something, but where is this used?
There was a problem hiding this comment.
it's not! good catch. removed.
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Motivation
This PR enables the owner of
GovernorHubto callrelayGovernancewith a new child messenger address, which will change themessengeraddress stored in theGovernorSpokeandOracleSpokeon the child chain. This allows the ChildMessenger to be upgraded without having to update the spoke contracts.Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.