🧱 Redesign to improve modularity - doc/spec

docs/adr/2022-01 Improving modularity of Mars.md

@@ -0,0 +1,213 @@
+# 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. 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:
+
+![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:
+
+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 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 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-Proxies.iuml)
+
+### 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.
+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<string> 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.
+
+### 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)
+
+![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'
+
+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'
+
+Abandoning futures, generating bindings and providing the bare utility: deployment, proxies, diffs, verification
+in a pluggable/extendable/composable fashion.
+
+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'
+
+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
+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

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).

docs/adr/uml/2022-01-Building-vs-executing.iuml

@@ -0,0 +1,102 @@
+@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)
+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
+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)
+autonumber resume
+Queue -> Exe: CONDITION_END
+autonumber stop
+end group
+
+@enduml

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

docs/adr/uml/2022-01-Proxies.iuml

@@ -0,0 +1,28 @@
+@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