Multi-chain reputation rebooted #1216
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
area
force-pushed
the
feat/multichain-reputation-rebased
branch
3 times, most recently
from
10860dc
to
1c38029
Compare
There was a problem hiding this comment.
Two high level comments:
-
I would be open to creating
ColonyNetworkSkillsin a separate PR which does nothing but re-arrange code. It'll make it easier to see what is actually changing behavior-wise in this one. -
There seems to be a lot of repetitive code in the bridging implementation. Overall I'd be happy seeing some more abstraction and helper functions introduced. Generalizing the low-level calls into some re-usable higher-level functions would help.
area
force-pushed
the
feat/multichain-reputation-rebased
branch
3 times, most recently
from
09a15be
to
f7c2511
Compare
area
changed the base branch from
develop
to
maint/introduce-colonyNetworkSkills
area
force-pushed
the
feat/multichain-reputation-rebased
branch
19 times, most recently
from
ad73979
to
0b4f19b
Compare
area
force-pushed
the
feat/multichain-reputation-rebased
branch
from
ebcaee3
to
8caed67
Compare
area
force-pushed
the
feat/multichain-reputation-rebased
branch
from
b449516
to
fb0f170
Compare
|
Included with the merge of #1237 |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A continuation / adaption of #1132. I didn't want to lose the contents of that branch, so this is a different branch based on it, with the bridging functionality changed and restructured.
Quite a big refactor since the last time someone other than me looked at this, so let's talk about the big change, which is the introduction of the
ColonyBridgecontract. The intention here is that this contract is the only contract thatcolonyNetworkinteracts with when it comes to bridging, and will do so for all time (a claim that will surely never become wrong) through an IColonyBridge interface. The intention here is to abstract out the specificities of any bridging solution we're using, and putting them all in the ColonyBridge contract.We had briefly talked about this being subsumed in to a single contract inside ColonyNetwork, but as I continued with implementation I ended up back here because of how we would need external calls anyway from ColonyNetwork to itself, as the bridging requests are spread across multiple files. May as well have the external calls to another contract. I also wasn't keen on polluting the ColonyNetwork contract storage with bridge-specific details. Either way, the upshot is that we no longer have the before/after bytes for each type of bridging transaction, and the behaviour is much easier to understand from reading the contracts (indeed, is actually defined by the contracts).
The ColonyBridge contract gets configured with all known ColonyBridges on other chains and the colonyNetwork on the chain it is deployed to. ColonyNetwork gets configured only with the ColonyBridge contract on its chain. ColonyBridges also get configured with whatever additional details need to be provided for the bridging solution being used. In this case, it's the wormhole address, but obviously could be anything with another implementation.
To change the bridge being used, we would deploy new sets of ColonyBridges to all chains with the new functionality, and then point the colonyNetworks at them - no changes to the ColonyNetwork contract required in that scenario. It would be nice to have a test with the old AMB functionality demonstrating that e.g. using the AMB we could still control a Gnosis Safe. I have confirmed with product that they are not expecting chain<->chain gnosis safe support with the multichain deployment, and so that functionality is going to be lost.
There is an implicit assumption here that colonyNetwork will only ever want to send messages to another colonyNetwork. I think that's fine, but I could see it meriting discussion.