From f6830ed210865c752bfef8ce8dfd56a8763e098a Mon Sep 17 00:00:00 2001 From: marcin Date: Mon, 17 Jan 2022 17:03:52 +0100 Subject: [PATCH 01/11] =?UTF-8?q?=F0=9F=A7=B1=20Redesign=20options=20doc?= =?UTF-8?q?=20to=20improve=20modularity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2022-01 Improving modularity of Mars.md | 171 ++++++++++++++++++ docs/adr/README.md | 6 + 2 files changed, 177 insertions(+) create mode 100644 docs/adr/2022-01 Improving modularity of Mars.md create mode 100644 docs/adr/README.md diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md new file mode 100644 index 0000000..37903e0 --- /dev/null +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -0,0 +1,171 @@ +# Improving modularity of Mars + +## Status + +Draft + +## Context + +### Modularity and too many if conditionals + +An attempt to introduce multisig (batched transactions) deployment into Mars uncovered areas of rigidity in its design. +Plugging in such an extension or feature should not lead to violating open-closed principle from +[SOLID](https://en.wikipedia.org/wiki/SOLID). In the attempt made significant number of existing code areas had to be +modified with conditional blocks which made the whole design overly complex (high code coupling). Therefore, discussion +started about an option to improve the design allowing for loose coupling and improved modularity. + +The ultimate goal is to add functionality as follows: + +```ts + +// old code + +deploy(() => { + const plumToken = contract('plum', Token, ['Plum', 'PLM']) + const tomatoToken = contract('tomato', Token, ['Tomato', 'TMT']) + contract(Market, [plumToken, tomatoToken]) +}) + +// adding a new feature - multisig deployments + +deploy(() => { + withMultisig(() => { + const plumToken = contract('plum', Token, ['Plum', 'PLM']) + const tomatoToken = contract('tomato', Token, ['Tomato', 'TMT']) + contract(Market, [plumToken, tomatoToken]) + }) +}) +``` + +And codebase internals should accept such a new feature in a simple manner e.g.: + +```ts +mars.deploymentTxCreator = new MultisigDeploymentTxCreator(config) +mars.txDispatcher = new MultisigTxDispatcher(config) +// so, it allows to replace particular behaviors with composable building blocks +``` + +### Main values of Mars so far + +1. Deployment of contracts from ABIs +2. Handling of proxies (deployment, upgrades, initialization) +3. Detecting diffs between developed in the repo contracts and those deployed previously to the network +4. Existence of Future type allows to pre-build the queue of steps (building phase) to be executed (execution phase) +5. Contract verification on Etherscan + +### Engine + +Currently, all the syntax like `contract(...)`, `createProxy(...)`, `proxy(...)`, `runIf(...)` works as follows: + +TODO: diagram showing 2 phases - building and executing -> with the queue and futures for completion of values + +### Proxies + +If a proxy detects its underlying implementation contract got changed, it tries to upgrade to the address of the new +implementation. There is a known issue (especially complex in multisig mode) when a proxy needs to know its current +implementation contract address and queues a read operation and eventually subsequent upgrade operation. In multisig + +Another all known proxy variants (EIP1967 or custom with parameterless constructor) are hardcoded into the proxy +whereas there should be composable/pluggable means to provide user defined proxy behavior (e.g. the strategy of +getting an underlying implementation). + +Below there is a lifecycle of proxy contract handling in Mars: + +TODO: diagram of proxy lifecycle (EIP and custom) + +### Futures + +The current documentations of Mars [states](https://ethereum-mars.readthedocs.io/en/latest/futures.html): + +> Futures allow the entire deployment script to be evaluated before the deployment itself happens. +> Futures are a declarative way to describe how values will be manipulated in the future. +> Intuitively you can think about them as “promises without await”. + +and + +> By removing the asynchronous evaluation we are able to identify a lot of issues with the script before anything is run. +> Suppose that the token didn’t have an approve method. In ethers we would only know after we deployed the contract +> and queried for the balance. However because Futures are synchronous the code token.approve(...) is run before any +> smart contract interaction, throwing an error early and saving us money. + +The main principle is to provide ability to build a structure of steps completely and only later execute in the network. +Compare: + +TODO: diagram showing Queue or AST vs sequential run + +There is a cost though -> it increases the complexity and reasoning about the flow of program execution. This pertains +to 2 development workflows: building the script and debugging its execution. + +Consider the following issue of conditional blocks: +``` +let currentImplementation = new Future(() => constants.AddressZero) +runIf(getCode(proxy[Address]).equals('0x').not(), () => { + + // NOTE!!! Regardless of the condition value in IF, the value of currentImplementation is set to + // Future of getImplementation(proxy) + // If the condition in IF is false, the currentImplementation is unresolved future instead of constants.AddressZero + currentImplementation = getImplementation(proxy) +}) +``` +For JS/TS programmers the above result is very unintuitive and leads to errors. + +It seems the same value could be achieved in a simpler way. First by generating the typed contract definitions +(via Mars future-free generate or typechain) and next to execute the script configuring a subset of behaviors: +- a run against in-memory Ganache or just collect transactions in the queue +- a run that visualizes the execution tree +- a run that verifies the contracts only +- a run that computes diffs between developed contracts in the repo and those already deployed + +Then the expression system does not mix with JS/TS language constructs and is very deterministic. +We then loose though the full structure of queue (or AST) containing all the steps that could be potentially applied to +the network. + +### Solution 'Simple' + +Abandoning futures, generating bindings and providing the bare utility: deployment, proxies, diffs, verification +in a pluggable/extendable/composable fashion. + +Modularity as follows: + +TODO: diagram + +Pros: +- very simple code structure and intuitive debugging -> nicer learning curve for newcomers +- easier to maintain extendable architecture and create extensions +Cons: +- without the complete tree of actions we cannot provide static-like analysis of execution + +#### How to get there + +1. Parametrize generation of artifacts and generate future free artifacts (or use typechain instead) +2. Extraction of static utility functions (when no state) or classes (when state is natural) for deployment, diffs, verification etc. +3. Abstracting away cross-cutting concerns like logging, console interaction or saving into files +4. Creating a new execution pipeline with extensions points (abstract strategies to plugged in like transaction dispatcher etc.) +5. Mirroring existing syntax (`contract`, `createProxy` etc.) (except `runIf`` conditionals as not needed) that +redirects to the new execution pipeline + +CLI should not be changed at first as we change only the underlying implementation. + +### Solution 'AST' + +#### How to get there + +### Solution 'Hybrid' + +The execution pipeline of solution 'Simple' can coexist with 'AST'. The common part would be the logical building blocks +and deployment script builders might choose one accordint to their need/preferences. + +Potentially 3 packages: +1. ethereum-mars-core +2. ethereum-mars +3. ethereum-mars-futures + +#### How to get there + +1. Follow Solution 'Simple' but build alongside (e.g. do not delete future-enabled generation process) +2. Build Solution 'AST' reusing the building blocks from point 1 +3. Setup package publishing + +## Decision + +## Consequences diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 0000000..3b7401e --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,6 @@ +# Architecture Decision Records + +This folder gathers all important decisions made to the project. +It follows a template described [here](https://github.com/joelparkerhenderson/architecture-decision-record/blob/main/templates/decision-record-template-by-michael-nygard/index.md). + +More on [Architecture Decision Records](https://github.com/joelparkerhenderson/architecture-decision-record). From c674992efa160577642fe0110a470c6cdea4e0d3 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 12:57:26 +0100 Subject: [PATCH 02/11] Add plant uml diagram, draft --- .../2022-01 Improving modularity of Mars.md | 13 ++- .../uml/2022-01 Building vs executing.iuml | 101 ++++++++++++++++++ 2 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 docs/adr/uml/2022-01 Building vs executing.iuml diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index 37903e0..3d3d77b 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -6,7 +6,7 @@ Draft ## Context -### Modularity and too many if conditionals +### Modularity and too many `if` conditionals An attempt to introduce multisig (batched transactions) deployment into Mars uncovered areas of rigidity in its design. Plugging in such an extension or feature should not lead to violating open-closed principle from @@ -17,9 +17,7 @@ started about an option to improve the design allowing for loose coupling and im The ultimate goal is to add functionality as follows: ```ts - // old code - deploy(() => { const plumToken = contract('plum', Token, ['Plum', 'PLM']) const tomatoToken = contract('tomato', Token, ['Tomato', 'TMT']) @@ -27,7 +25,6 @@ deploy(() => { }) // adding a new feature - multisig deployments - deploy(() => { withMultisig(() => { const plumToken = contract('plum', Token, ['Plum', 'PLM']) @@ -50,14 +47,16 @@ mars.txDispatcher = new MultisigTxDispatcher(config) 1. Deployment of contracts from ABIs 2. Handling of proxies (deployment, upgrades, initialization) 3. Detecting diffs between developed in the repo contracts and those deployed previously to the network -4. Existence of Future type allows to pre-build the queue of steps (building phase) to be executed (execution phase) -5. Contract verification on Etherscan +4. Contract verification on Etherscan +5. Existence of Future type allows to pre-build the queue of steps (building phase) to be executed (execution phase) later + +Existence of `Future` construct is not mandatory for all the other functionality - it offers an added value though. ### Engine Currently, all the syntax like `contract(...)`, `createProxy(...)`, `proxy(...)`, `runIf(...)` works as follows: -TODO: diagram showing 2 phases - building and executing -> with the queue and futures for completion of values +[Sequence diagram](./uml/2022-01%20Building%20vs%20executing.iuml) ### Proxies diff --git a/docs/adr/uml/2022-01 Building vs executing.iuml b/docs/adr/uml/2022-01 Building vs executing.iuml new file mode 100644 index 0000000..c7c774f --- /dev/null +++ b/docs/adr/uml/2022-01 Building vs executing.iuml @@ -0,0 +1,101 @@ +```plantuml +@startuml +actor Developer as Dev +participant "Contracts repo" as Repo +participant "Mars" as Mars +participant "Mars script" as Script +queue "Actions queue" as Queue +control "Execution engine" as Exe +boundary "Network" as Network + +group Writing for the first deployment +Dev -> Repo: Write Token.sol +Dev -> Repo: Write MyProxy.sol (EIP1967) +Dev -> Repo: Generate JSON ABIs +Dev -> Mars: Generate artifacts +note left +Artifacts are Future-enabled +version of ethers contracts +end note +Dev -> Script: Write deployment script +note left +// ... +const token = contract(Token) +const proxy = createProxy(MyProxy) +runIf(token.getManager().equals(constants.AddressZero), () => { + proxy.setManager(myFirstManagerAddress) +}) +// ... +end note +end group + +group Deploying the first time +Dev -> Script: Run deployment providing all config +autonumber +note over Script, Queue: const token = contract(Token) +Script -> Queue: DEPLOY of Token +note right #aqua: ( ) => future Token address +note over Script, Queue: const proxy = createProxy(MyProxy) +Script -> Queue: DEPLOY of MyProxy +note right #aqua: (future Token address) = > future MyProxy address +Script -> Queue: READ MyProxy current impl +note right #aqua: (future MyProxy address) => future MyProxy current impl +Script -> Queue: CONDITION_START (run if impl changed) +note right #aqua: (future MyProxy current impl) => future boolean +Script -> Queue: TRANSACT Upgrade to new impl address +note right #aqua: (future MyProxy address) => { } +Script -> Queue: CONDITION_END +note over Script, Queue +runIf(token.getManager().equals(constants.AddressZero), () => { + proxy.setManager(myFirstManagerAddress) +}) +end note +Script -> Queue: READ Check manager address +note right #aqua: (future MyProxy address) => future Manager address +Script -> Queue: CONDITION_START (run if no manager) +note right #aqua: (future Manager address) => future boolean +Script -> Queue: TRANSACT Set manager to some address +note right #aqua: (future Manager address) => { } +Script -> Queue: CONDITION_END +autonumber stop +Script -> Exe: Run actions +note over Queue, Exe: A primitive visitor pattern processing AST +Exe -> Queue: Get actions for processing +autonumber +Queue -> Exe: DEPLOY of Token +autonumber stop +Exe -> Network: Deploy Token contract +note left #lime: resolve future Token address +autonumber resume +Queue -> Exe: DEPLOY of MyProxy +autonumber stop +Exe -> Network: Deploy MyProxy contract with impl addr +note left #lime: resolve future MyProxy address +autonumber resume +Queue -> Exe: READ MyProxy current impl +autonumber stop +Exe -> Network: Get impl address +Network -> Exe: 0x13bff3... (= current impl) +note left #lime: resolve future MyProxy current impl +autonumber resume +note over Queue, Exe: Skipping conditional block as no impl changed +Queue -> Exe: CONDITION_START (run if impl changed) +Queue -> Exe: TRANSACT Upgrade to new impl address +Queue -> Exe: CONDITION_END +note over Queue, Exe: Setting a new manager +Queue -> Exe: READ Check manager address +autonumber stop +Exe -> Network: Get manager address +Network -> Exe: 0x000... (= none) +autonumber resume +Queue -> Exe: CONDITION_START (run if no manager) +Queue -> Exe: TRANSACT Set manager to some address +autonumber stop +Exe -> Network: Set manager to 0x24cba... (= current) +autonumber resume +Queue -> Exe: CONDITION_END +autonumber stop +end group + +@enduml +``` From 34fd0267b148713a85c11ab9e52767649370ef8a Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 21:35:23 +0100 Subject: [PATCH 03/11] Add plant uml building vs executing, v2 --- docs/adr/uml/2022-01 Building vs executing.iuml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/adr/uml/2022-01 Building vs executing.iuml b/docs/adr/uml/2022-01 Building vs executing.iuml index c7c774f..86dee10 100644 --- a/docs/adr/uml/2022-01 Building vs executing.iuml +++ b/docs/adr/uml/2022-01 Building vs executing.iuml @@ -80,6 +80,7 @@ note left #lime: resolve future MyProxy current impl autonumber resume note over Queue, Exe: Skipping conditional block as no impl changed Queue -> Exe: CONDITION_START (run if impl changed) +note left #lime: resolve future boolean Queue -> Exe: TRANSACT Upgrade to new impl address Queue -> Exe: CONDITION_END note over Queue, Exe: Setting a new manager @@ -87,8 +88,10 @@ Queue -> Exe: READ Check manager address autonumber stop Exe -> Network: Get manager address Network -> Exe: 0x000... (= none) +note left #lime: resolve future Manager address autonumber resume Queue -> Exe: CONDITION_START (run if no manager) +note left #lime: resolve future boolean Queue -> Exe: TRANSACT Set manager to some address autonumber stop Exe -> Network: Set manager to 0x24cba... (= current) From e2484716d5b85b8d5325d924b3289f9e7eacca84 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:22:13 +0100 Subject: [PATCH 04/11] Add Proxies lifecycle diagram --- docs/adr/uml/2022-01 Proxies.iuml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 docs/adr/uml/2022-01 Proxies.iuml diff --git a/docs/adr/uml/2022-01 Proxies.iuml b/docs/adr/uml/2022-01 Proxies.iuml new file mode 100644 index 0000000..58a0e37 --- /dev/null +++ b/docs/adr/uml/2022-01 Proxies.iuml @@ -0,0 +1,30 @@ +```plantuml +@startuml +partition "Proxy lifecycle" { +start +:Deploy logic contract: Token; +note left +const token = __**contract(Token)**__ +end note +:Deploy proxy contract of Token logic: Token_proxy; +note left +const proxy = createProxy(MyProxyContract, 'upgradeTo') +const proxiedToken = __**proxy(token**__, 'init', [123]) +end note +:Upgrade to: Token; +note left +const proxy = createProxy(MyProxyContract, __**'upgradeTo'**__) +const proxiedToken = proxy(__**token**__, 'init', [123]) +end note +:Initialize: run only once 'init' with params '[123]'; +note left +const proxy = createProxy(MyProxyContract, 'upgradeTo') +const proxiedToken = proxy(token, __**'init', [123]**__) +end note + +repeat :A developer changes implementation logic Token; + :Upgrade proxy implementation to new token address; +repeat while (awaiting changes in implementation logic) + +@enduml +``` From 171867962d32437980c70b57b498d46d95c265f1 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:22:38 +0100 Subject: [PATCH 05/11] Editing adr --- .../2022-01 Improving modularity of Mars.md | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index 3d3d77b..edeff39 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -58,19 +58,38 @@ Currently, all the syntax like `contract(...)`, `createProxy(...)`, `proxy(...)` [Sequence diagram](./uml/2022-01%20Building%20vs%20executing.iuml) +There are 2 significant parts: + +1. Queueing actions and their execution are separated. This allows for complete build up of actions to be instantiated. +2. This requires introducing structs of values that we can declare in build-up time and evaluate later (Futures). + +In the above diagram aqua boxes describes creation of such structs (Futures), specifically `(future A) => future B` +tells that the given step depends on availability of value A in the future and resolves the value of B when the step +is completed successfully. + +If we remove actions queue and execution engine then are left with direct calls to the network which is very simple +However, during script execution not every action path would be built. + ### Proxies If a proxy detects its underlying implementation contract got changed, it tries to upgrade to the address of the new implementation. There is a known issue (especially complex in multisig mode) when a proxy needs to know its current implementation contract address and queues a read operation and eventually subsequent upgrade operation. In multisig +if the proxy we cannot read from non-existing proxy (imagine queuing such a read just after proxy creation landed +in multisig batch and not yet in the network). We need to read if the proxy has been created and only then read +the current implementation. This adds conditional complexity to the proxy handling which is especially entangled +combining with Future structures. -Another all known proxy variants (EIP1967 or custom with parameterless constructor) are hardcoded into the proxy -whereas there should be composable/pluggable means to provide user defined proxy behavior (e.g. the strategy of -getting an underlying implementation). +Another problem stems from the fact that all the known proxy variants (EIP1967 or custom) are hardcoded into the proxy +whereas proxying in Mars should offer also: +- replacing the way the current implementation contract address is obtained +- providing an easy way to specify ctor param values (EIP1967) for a specific implementation contract +- constructing a user defined proxy built with some behaviors of Mars default proxy but adding also more (e.g. automatic +ownership transferring or interacting with the old version of the proxy/logic just before upgrade to the new one) -Below there is a lifecycle of proxy contract handling in Mars: +Below there is a lifecycle of proxy contracts handling in Mars: -TODO: diagram of proxy lifecycle (EIP and custom) +[Activity diagram](./uml/2022-01%20Proxies.iuml) ### Futures @@ -88,10 +107,6 @@ and > smart contract interaction, throwing an error early and saving us money. The main principle is to provide ability to build a structure of steps completely and only later execute in the network. -Compare: - -TODO: diagram showing Queue or AST vs sequential run - There is a cost though -> it increases the complexity and reasoning about the flow of program execution. This pertains to 2 development workflows: building the script and debugging its execution. @@ -119,15 +134,17 @@ Then the expression system does not mix with JS/TS language constructs and is ve We then loose though the full structure of queue (or AST) containing all the steps that could be potentially applied to the network. +### Improving modularity + +### Solution 'Modular step by step' + + + ### Solution 'Simple' Abandoning futures, generating bindings and providing the bare utility: deployment, proxies, diffs, verification in a pluggable/extendable/composable fashion. -Modularity as follows: - -TODO: diagram - Pros: - very simple code structure and intuitive debugging -> nicer learning curve for newcomers - easier to maintain extendable architecture and create extensions @@ -140,7 +157,7 @@ Cons: 2. Extraction of static utility functions (when no state) or classes (when state is natural) for deployment, diffs, verification etc. 3. Abstracting away cross-cutting concerns like logging, console interaction or saving into files 4. Creating a new execution pipeline with extensions points (abstract strategies to plugged in like transaction dispatcher etc.) -5. Mirroring existing syntax (`contract`, `createProxy` etc.) (except `runIf`` conditionals as not needed) that +5. Mirroring existing syntax (`contract`, `createProxy` etc.) (except `runIf` conditionals as not needed) that redirects to the new execution pipeline CLI should not be changed at first as we change only the underlying implementation. From cd22ecb17cf10f3c322695428b0d50a7b86c0571 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:29:06 +0100 Subject: [PATCH 06/11] Fix plant uml links To be rendered in GH Markdown After https://github.com/jonashackt/plantuml-markdown hints --- docs/adr/2022-01 Improving modularity of Mars.md | 4 ++-- docs/adr/uml/2022-01 Building vs executing.iuml | 2 -- docs/adr/uml/2022-01 Proxies.iuml | 2 -- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index edeff39..50254a0 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -56,7 +56,7 @@ Existence of `Future` construct is not mandatory for all the other functionality Currently, all the syntax like `contract(...)`, `createProxy(...)`, `proxy(...)`, `runIf(...)` works as follows: -[Sequence diagram](./uml/2022-01%20Building%20vs%20executing.iuml) +![Building vs executing](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01%20Building%20vs%20executing.iuml) There are 2 significant parts: @@ -89,7 +89,7 @@ ownership transferring or interacting with the old version of the proxy/logic ju Below there is a lifecycle of proxy contracts handling in Mars: -[Activity diagram](./uml/2022-01%20Proxies.iuml) +![Proxies lifecycle](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01%20Proxies.iuml) ### Futures diff --git a/docs/adr/uml/2022-01 Building vs executing.iuml b/docs/adr/uml/2022-01 Building vs executing.iuml index 86dee10..d224dc3 100644 --- a/docs/adr/uml/2022-01 Building vs executing.iuml +++ b/docs/adr/uml/2022-01 Building vs executing.iuml @@ -1,4 +1,3 @@ -```plantuml @startuml actor Developer as Dev participant "Contracts repo" as Repo @@ -101,4 +100,3 @@ autonumber stop end group @enduml -``` diff --git a/docs/adr/uml/2022-01 Proxies.iuml b/docs/adr/uml/2022-01 Proxies.iuml index 58a0e37..1d63e60 100644 --- a/docs/adr/uml/2022-01 Proxies.iuml +++ b/docs/adr/uml/2022-01 Proxies.iuml @@ -1,4 +1,3 @@ -```plantuml @startuml partition "Proxy lifecycle" { start @@ -27,4 +26,3 @@ repeat :A developer changes implementation logic Token; repeat while (awaiting changes in implementation logic) @enduml -``` From cebca8123b59be0c28388e8ae2ed038434ed91d9 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:48:20 +0100 Subject: [PATCH 07/11] Fixing PUML generation via proxy --- docs/adr/2022-01 Improving modularity of Mars.md | 2 +- docs/adr/uml/{2022-01 Proxies.iuml => 2022-01-Proxies.iuml} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename docs/adr/uml/{2022-01 Proxies.iuml => 2022-01-Proxies.iuml} (100%) diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index 50254a0..ab271f3 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -89,7 +89,7 @@ ownership transferring or interacting with the old version of the proxy/logic ju Below there is a lifecycle of proxy contracts handling in Mars: -![Proxies lifecycle](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01%20Proxies.iuml) +![Proxies lifecycle](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01-Proxies.iuml) ### Futures diff --git a/docs/adr/uml/2022-01 Proxies.iuml b/docs/adr/uml/2022-01-Proxies.iuml similarity index 100% rename from docs/adr/uml/2022-01 Proxies.iuml rename to docs/adr/uml/2022-01-Proxies.iuml From 0831f0d68635e1cef41e2b0cd8518b31f5bb08a5 Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:52:15 +0100 Subject: [PATCH 08/11] Fixing PUML generation via proxy --- ...lding vs executing.iuml => 2022-01-Building-vs-executing.iuml} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/adr/uml/{2022-01 Building vs executing.iuml => 2022-01-Building-vs-executing.iuml} (100%) diff --git a/docs/adr/uml/2022-01 Building vs executing.iuml b/docs/adr/uml/2022-01-Building-vs-executing.iuml similarity index 100% rename from docs/adr/uml/2022-01 Building vs executing.iuml rename to docs/adr/uml/2022-01-Building-vs-executing.iuml From 4a1a596bb4bb03a1a6b50e5bf95e6f3bcdc3851b Mon Sep 17 00:00:00 2001 From: marcin Date: Tue, 18 Jan 2022 23:52:15 +0100 Subject: [PATCH 09/11] Fixing PUML generation via proxy --- docs/adr/2022-01 Improving modularity of Mars.md | 2 +- ...ing vs executing.iuml => 2022-01-Building-vs-executing.iuml} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename docs/adr/uml/{2022-01 Building vs executing.iuml => 2022-01-Building-vs-executing.iuml} (100%) diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index ab271f3..c09f054 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -56,7 +56,7 @@ Existence of `Future` construct is not mandatory for all the other functionality Currently, all the syntax like `contract(...)`, `createProxy(...)`, `proxy(...)`, `runIf(...)` works as follows: -![Building vs executing](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01%20Building%20vs%20executing.iuml) +![Building vs executing](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01-Building-vs-executing.iuml) There are 2 significant parts: diff --git a/docs/adr/uml/2022-01 Building vs executing.iuml b/docs/adr/uml/2022-01-Building-vs-executing.iuml similarity index 100% rename from docs/adr/uml/2022-01 Building vs executing.iuml rename to docs/adr/uml/2022-01-Building-vs-executing.iuml From 36ae74dbc9d8eaca0289a24ed4a87514bcd11c1e Mon Sep 17 00:00:00 2001 From: marcin Date: Wed, 19 Jan 2022 12:12:10 +0100 Subject: [PATCH 10/11] [ADR] More on modularity and possible solutions --- .../2022-01 Improving modularity of Mars.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index c09f054..7034870 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -136,9 +136,27 @@ the network. ### Improving modularity +Regardless of keeping Futures or not, we want Mars be composable/extendable in 2 ways: + +1. It should offer basic utility building blocks (e.g. contract diff, deployments, proxies) to be used to construct custom +pieces (custom proxies, deployment schemes etc.) +2. It should offer extension points (e.g. plugging in a specific strategy e.g. multisig batching and execution) + + + ### Solution 'Modular step by step' +Note! This solution is orthogonal to other solutions below. It addresses modularity problem and other solutions address +Future struct complexity. It is recommended to pick this solution + any of the solutions below. +1. Extracting transaction dispatching and abstract away behind some gateway. +2. Extract contract creation strategy (CREATE vs CREATE2) and make it configurable/replaceable. +3. Extract deployments file reading/saving. +4. Extract existing deployment information retrieval and provision it for actions building phase (now it is available +only in the execution phase). +5. **Implement multisig module as extension to the main process.** +6. **Make sure multisig module abstracts away its particular implementation, e.g. Gnosis Safe** +7. Extract verification and abstract away etherscan verification API behind a gateway ### Solution 'Simple' @@ -164,8 +182,16 @@ CLI should not be changed at first as we change only the underlying implementati ### Solution 'AST' +Replacing actions queue with Abstract Syntax Tree and executeX functions with composable visitors should improve +the overall design by make it more loosely coupled, thus extendable without the risk of modification of existing parts. + #### How to get there +1. Improve modularity first, see Solution Modular +2. Replace actions queue with AST where `createProxy` syntax node consists of smaller building blocks like DEPLOY, READ, +UPGRADE_TO. This is useful to represent block wrappers like `runIf` or `withMultisig`. +3. Replace `executeX` functions with composable visitors (each traversing the tree and doing one specific job). + ### Solution 'Hybrid' The execution pipeline of solution 'Simple' can coexist with 'AST'. The common part would be the logical building blocks From 88ad7682fa6dca0ff0b41e04e2927fb4643bc9e3 Mon Sep 17 00:00:00 2001 From: marcin Date: Thu, 20 Jan 2022 12:17:46 +0100 Subject: [PATCH 11/11] [ADR] More on modularity - diagram --- .../2022-01 Improving modularity of Mars.md | 2 +- docs/adr/uml/2022-01-Modularity.iuml | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 docs/adr/uml/2022-01-Modularity.iuml diff --git a/docs/adr/2022-01 Improving modularity of Mars.md b/docs/adr/2022-01 Improving modularity of Mars.md index 7034870..2c0a341 100644 --- a/docs/adr/2022-01 Improving modularity of Mars.md +++ b/docs/adr/2022-01 Improving modularity of Mars.md @@ -142,7 +142,7 @@ Regardless of keeping Futures or not, we want Mars be composable/extendable in 2 pieces (custom proxies, deployment schemes etc.) 2. It should offer extension points (e.g. plugging in a specific strategy e.g. multisig batching and execution) - +![Modularity](http://www.plantuml.com/plantuml/proxy?cache=no&src=https://raw.githubusercontent.com/EthWorks/Mars/marcin/redesign-doc/docs/adr/uml/2022-01-Modularity.iuml) ### Solution 'Modular step by step' diff --git a/docs/adr/uml/2022-01-Modularity.iuml b/docs/adr/uml/2022-01-Modularity.iuml new file mode 100644 index 0000000..64ece46 --- /dev/null +++ b/docs/adr/uml/2022-01-Modularity.iuml @@ -0,0 +1,35 @@ +@startuml +class Mars +Mars : new (behaviors: MarsBehaviors) +interface MarsBehaviors +MarsBehaviors : networkState: NetworkState +MarsBehaviors : contractCreator: ContractCreator +MarsBehaviors : transactionDispatcher: TransactionDispatcher +MarsBehaviors : ... + +abstract NetworkState +class FileNetworkState +NetworkState <|-- FileNetworkState + +abstract ContractVerifier +class EtherscanContractVerifier +ContractVerifier <|-- EtherscanContractVerifier + +abstract ContractCreator +class BasicContractCreator +class Create2ContractCreator +ContractCreator <|-- BasicContractCreator +ContractCreator <|-- Create2ContractCreator + +abstract TransactionDispatcher +class InstantTransactionDispatcher +class MultisigTransactionDispatcher +TransactionDispatcher <|-- InstantTransactionDispatcher +TransactionDispatcher <|-- MultisigTransactionDispatcher + +interface Proxy +class CustomProxy +class Eip1967Proxy +Proxy <|-- CustomProxy +Proxy <|-- Eip1967Proxy +@enduml