feat: contract deployment and upgrades writeup

[AuHau]

Writeup about codex-storage/codex-pm#51

[emizzle]

Thanks for doing this, Adam. Very good to have all of this discussion and knowledge captured.

Regarding contract modularisation, I have a few questions/comments:

1. From our initial discussion, I thought there would be a third contract that deals primarily with holding the StorageRequest mappings, so that the business logic and funds contracts could be upgraded without wiping the network? What was the reasoning behind leaving this out?
2. The TimeVault contract will extend ERC20 and manage the spending of tokens, correct? If so, it may be worth mentioning.
3. We initially discussed that we'd try to limit the amount of funds that could be sent to any one address by utilising an ACL of permissible recipient addresses for a deposit. Most funds could be locked to a single address, and repair rewards could be sent to any address (*), which is better than allowing all funds to be sent to any address. However, the proposed API uses fromAddress only. Would you mind explaining the reasoning? My suspicion is that this leverages the ERC20 transferFrom and approve functions, but IMO, that is not enough to prevent a hack from draining funds, especially given that we have already discussed adding infinite approval, meaning that the caller in transferFrom can transfer an infinite amount of funds. I do prefer the simplicity of the outlined approach over the ACL, however, it seems there's not as much protection.
4. We also discussed utilising OpenZeppelin's proxy contracts for the modular contracts approach. This would enable upgrading each of the three(?) contracts, while retaining only one Marketplace address in the Codex client. However, at the start of the writeup, it is mentioned that proxy contracts provides too much power to the admin role. The admin role would have permission to update the proxy contract, but not force a user to use a specific version, as we discussed passing a contract version parameter, see next point.
5. There was also discussion of passing in a version parameter from the client to the proxy contract, which in my opinion is a good way to retain the users' control over which contract to call, in the same way that having a single Marketplace contract in the client does. When one of the three(?) contracts are upgraded, a new "contracts version" is created, and the Codex client can be updated with this contracts version. The version would be passed from the Codex client to the proxy contract for each call, thus enabling users to use whichever version of the contract they desire. Users on old versions would still use the old version without issue. As you mentioned in the writeup, the limited contract duration would prevent outdated contract versions from existing for too long. This is a downside for extending the max contract duration too long.

As a general comment, perhaps you could look at checking your writing with a grammar and spell check plugin?

[emizzle]

I thought that having modular funds contract separated out would allow re-deployment of only the funds contract on its own?

[AuHau]

If you have compromised the vault in a way that somebody can drain its funds and you freeze it because of it, then you deploy new fixed version - what do you do about the funds freezed in the original vault? You can not unfreeze it to "move it to the new one" because the attacker could exploit the bug because of which it was freezed for.

Dmitriy's suggestion in our session was to do a "community consensus" hard-fork of the token. Where new token would be deployed, holders of the existing tokens could redeem the new tokens for the old ones and the funds in the old vault would be replaced in the new fixed vault by mint of the new tokens.

[AuHau]

From our initial discussion, I thought there would be a third contract that deals primarily with holding the StorageRequest mappings, so that the business logic and funds contracts could be upgraded without wiping the network? What was the reasoning behind leaving this out?

This was proposed mainly by you at the beginning of the exploratory work on this, but I don't think it was mentioned during our design session in Lisbon. We do not need this part because we will have "upgrade" capabilities on the Marketplace contract, so from an emergency upgrades perspective, we won't wipe the network when we need to patch something. And for the normal feature upgrades, we will still follow the original upgrade process through deprecation of old contracts and deploying new contracts.

The TimeVault contract will extend ERC20 and manage the spending of tokens, correct? If so, it may be worth mentioning.

While they share some similarities - no, TimeVault won't extend ERC20.

We initially discussed that we'd try to limit the amount of funds that could be sent to any one address by utilising an ACL of permissible recipient addresses for a deposit. Most funds could be locked to a single address, and repair rewards could be sent to any address (*), which is better than allowing all funds to be sent to any address. However, the proposed API uses fromAddress only. Would you mind explaining the reasoning? My suspicion is that this leverages the ERC20 transferFrom and approve functions, but IMO, that is not enough to prevent a hack from draining funds, especially given that we have already discussed codex-storage/nim-codex#598, meaning that the caller in transferFrom can transfer an infinite amount of funds. I do prefer the simplicity of the outlined approach over the ACL, however, it seems there's not as much protection.

Already in our designing session we have scratched the "recipient based locking" as I have described in the document:

Unfortunately, this concept is not applicable for Marketplace because of slot repairs, when one slot's host is replaced
with another, which would require reallocating funds and hence open an opportunity for hackers to redirect the funds to
their accounts.

The fromAccount parameter is here for different reasons. As this will be a stand-alone contract anybody can call it - people, other contracts, etc. fromAccount in the context of TimeVault will be actually the address of the Marketplace contract as that should be the only account that can manipulate the funds that it "locks" into this vault.
But this is good feedback - I will elaborate on this point in the document.

We also discussed utilising OpenZeppelin's proxy contracts for the modular contracts approach. This would enable upgrading each of the three(?) contracts, while retaining only one Marketplace address in the Codex client. However, at the start of the writeup, it is mentioned that proxy contracts provides too much power to the admin role. The admin role would have permission to update the proxy contract, but not force a user to use a specific version, as we discussed passing a contract version parameter, see next point.

That is from the original document when we discussed the previous solutions. This point still stands because, as a solution, this is not passable. Only with modularisation of the contracts and splitting up the funds-keeping part into a vault in a separate contract can we actually use it for the Marketplace contract. I will review this section and its wording together with adding a note to the new section about why we can use the Upgradable concept with modularization.

There was also discussion of passing in a version parameter from the client to the proxy contract, which in my opinion is a good way to retain the users' control over which contract to call, in the same way that having a single Marketplace contract in the client does. When one of the three(?) contracts are upgraded, a new "contracts version" is created, and the Codex client can be updated with this contracts version. The version would be passed from the Codex client to the proxy contract for each call, thus enabling users to use whichever version of the contract they desire. Users on old versions would still use the old version without issue. As you mentioned in the write-up, the limited contract duration would prevent outdated contract versions from existing for too long. This is a downside for extending the max contract duration too long.

I don't recall this, but generally, the "upgradability" of contracts is still reserved only for emergency/critical bug-fixing cases. In other cases the "feature upgrade" still should be used. In this way, the "versioning" is performed by the node itself, as it will monitor multiple contract versions running in parallel and submit new requests only to the newest one. In this way, we will be upgrading only the Marketplace contract, and the Vault should be persistent unless there is a critical bug fund in it in which the "emergency freezing" of it will be invoked.

As a general comment, perhaps you could look at checking your writing with a grammar and spell check plugin?

Thanks for the suggestion 😅 I was using some freemium spell-checker, but now I got myself a Grammarly subscription so hopefully the level of my grammar should improve 😅

[AuHau]

@emizzle after today's discussion WDYT? Can you give it another review?
@markspanbroek could you please give it review as well?

[emizzle]

I'm still a bit confused after reading this proposal 😅

Maybe there could be some more details in there to provide some clarity around the latest proposal that combines ideas of previous proposals, particularly:

1. The TimeVault and Token contracts are passed in as constructor arguments to the Marketplace contract. The codex client contains only the address of the marketplace contract (hardcoded? cli param?), which, after upgrading one of the three contracts (token, marketplace, timevault), would require an update and release of the client.
2. A small paragraph about what would happen in the case of an emergency. In particular, which contracts would be affected and how would the admin interact with them. Also, it seems we are missing the idea of re-deploying the Token contract and then redeeming old tokens for new tokens at a pre-hack snapshot.
3. A small paragraph about what would happen in the case of a normal upgrade, which contracts would be affected, and how the client would be affected.
4. A small paragraph about the situations that would wipe the network

[emizzle]

Here's a point of confusion for me. The Upgradable Contract section describes using proxy contracts to upgrade. After the discussion we just had, I was under the impression we wouldn't be using proxy contracts (I understand i was proposing to use it for both contracts, not just one), but after re-reading this, I think you are trying to say that the TimeVault contract would be controlled by a proxy contract to allow for quick emergency upgrades. However, in the next paragraph, you say "This contract would not incorporate the upgradibility capabilities". What are the "upgradability capabilities" referring to?

I think there are two things that should be clarified here:

1. The terminology is very confusing: "Upgradable contract" vs "Upgrades". Imo, these need to be renamed to avoid confusion, maybe be something like "Upgrades via proxy contract" and "Contract replacement upgrades"
2. Is the TimeVault contract using a proxy contract or not?

I think maybe these two paragraphs should be carefully rewritten if possible to provide maximum clarity. It's ok to reiterate the solutions proposed in previous sections, as this is more clear for the reader than having them make assumptions based on previous sections.