Factory manages contract versions

ilblackdragon commented 2 years ago

Generalize factory to store multiple version of the contracts. DAOs can just pull the specific version of the code instead requiring to upload it directly into their contract.

Adding ability for factory to "push" upgrade for a specific DAO if it is enabled.

ctindogaru commented 2 years ago

Finished testing this PR in https://github.com/near-daos/sputnik-dao-contract/pull/76

Didn't find any issues apart from the one mentioned.

ctindogaru commented 2 years ago

Had some discussions on this and there are some concerns on the upgrade.

How it works now is that you store a key/value pair such that:

the key is the sha256 hash of the value
the value is the wasm file represented as a vector of bytes

Concerns:

how do you know what that wasm file represents? Since it's just an array of bytes, you have no way of telling what fixes/features it includes and to what snapshot of the code it points to
how do you know how many versions does the factory store and how do you choose the right version?

Suggestions:

besides the vector of bytes, we should store some metadata that explains what the wasm file represents: snapshot of the contract code in the time (ex: commit id), version number: V1/V2/V3, description, github link or any other useful information.
provide a way to get all the available versions so the users can pick which versions suits the best for them (ex: get_contract_versions will return [v1: commit_id1, v1.1: commit_id2, v2: commit_id3, v2.1: commit_id4, v3: commit_id5)

ilblackdragon commented 2 years ago

Hm, it's surprising that everything worked given the contract storing method has assert in the opposite direction: https://github.com/near-daos/sputnik-dao-contract/pull/69/files#diff-3dd4449e06b690d9a171047ec7a41be850335de5eaa66aa192a71ac7880c1e24R155

ctindogaru commented 2 years ago

Hm, it's surprising that everything worked given the contract storing method has assert in the opposite direction: https://github.com/near-daos/sputnik-dao-contract/pull/69/files#diff-3dd4449e06b690d9a171047ec7a41be850335de5eaa66aa192a71ac7880c1e24R155

This is so nasty. It's my bad, it got away because I didn't attach any deposit to the call when testing: https://github.com/near-daos/sputnik-dao-contract/pull/76/files#diff-466303b3345b955ec8bba077825b0b9ead3fee8ce71d2959b32c80965048f116R52-R55

Good spot.

ilblackdragon commented 2 years ago

That's on me - I didn't have time to write tests for the factory which are really needed here.

If you have time this would really use good integration test for factory upgrade which would also have showcased the other issue you have found.

ilblackdragon commented 2 years ago

RE: adding meta data -- that's a very good point. But a bit complicated in the case of storing contract, as that method doesn't parse arguments to be optimal on gas - just stores the whole blob of arguments into storage.

Potentially can add a separate method which allows to store metadata of the given code hash.

@ctindogaru how about merge this in and then you can add separate PR that would store metadata for the code hash?

ctindogaru commented 2 years ago

Was thinking the same thing today. I don't want to make this PR bigger than it needs to be.

Since all the known issues have been resolved, will take another careful look tomorrow and probably merge it as it stands. Then, I will try to come up with 2 PRs: one for metadata, one for testing.

If we keep adding to this PR, it will get too complex and it will become harder to review. Smaller increments over it is better.

near-daos / sputnik-dao-contract

Factory manages contract versions #69