search

near-daos / sputnik-dao-contract

Smart contracts for https://app.astrodao.com
https://astrodao.com/
MIT License
108 stars 81 forks source link

Change ProposalKind::UpgradeSelf to call the factory for fetching the wasm code #105

Closed ctindogaru closed 2 years ago

ctindogaru commented 2 years ago

DAOs no longer need to store the wasm code required for the upgrade inside their storage.

Instead, they will call the factory and get the wasm code from there.

By making the code only available in the factory:

ctindogaru commented 2 years ago

testing not yet done. working on it

ctindogaru commented 2 years ago

@ctindogaru good to see the progress here! A couple architectural thoughts/questions for you:

  1. Following the model of the DAO submitting the code_hash, potentially the pitfall becomes a proposal that upgrades past where it can safely upgrade to (sans migrations, etc). I feel the DAO could accidentally skip or lie about the version it should upgrade to and potentially corrupt the DAO irreversibly.
  2. Versioning - Along the same thread as above, I believe the "V2", "V3" model may not work in our favor for long.

Considering the above items, what are your thoughts on:

  1. The factory keeps track of upgrade path, where the DAO just says "upgrade me" and the Factory has enough context to process the upgrade safely. A -> B -> C, rather than DAO saying: A -> C. I know it would also simplify the client UX as well.
  2. To make above happen, Factory could implement semver, or simplified semver, so the factory could easily make decisions about next logical upgrade code_hash. It would also ensure no backward semi-unsafe downgraded code_hash.

I have been trying to formalize this in the document: https://github.com/near-daos/sputnik-dao-contract/wiki/WIP:-DAO-Factory-&-Registry What are your thoughts on this path instead?

Very good observations Trevor.

I completely agree with A -> B -> C approach and we should restrict any A -> C type of upgrades. This is because A -> B may need some data migration (that's automatically handled by the system), but B -> C does not need any data migration. So if you're doing A -> C, the data migration from A to B is skipped and your DAO may end up in an unpredictable state.

What it needs more thinking is:

Anther thought:

TrevorJTClarke commented 2 years ago

What it needs more thinking is:

TC: Adding inline thoughts to each

do we support downgrades? We can handle data migrations from A to B, but B -> A is not possible. So i'm inclined towards saying no here.

TC: I really dont like the idea of downgrades. It introduces a lot of bad storage and potential attack surface. If a DAO needed a downgrade, we could suggest that they do a manual data migration to a DAO created by an earlier version. Which kinda brings up the idea of allowing creating a DAO at a specific version, which might be nice if there was demand and a solid reason? haha

where do we store the versioning for each DAO? In the factory? In the DAO? External database? Nowhere and is it handled by the UI?

TC: I would like to explore the factory having more knowledge of the versions & DAO, it would require a little more cost for storage, but could allow for easier maintenance and improvements over time. The downside is more of a "centralized" point of failure, unless a DAO pointed to a different factory in the future. Since we're storing a list of all DAOs in the factory, could change to mapping instead. Could go both ways on this. The mapping would then be: UnorderedMap<AccountId, u8> or similar

what versioning system are we using? counter? string? semver? The only problem that I have with using semver is the syntax, that may be very useful for certain applications, but doesn't seem very useful for sputnik: major.minor.patch.

TC: Very good points on this - went down that path a little in my head, but liked your summary. We could likely keep to a single uint then, i just dont like the idea of String being the version. It could get messy to programmatically check version stance in the future. I like the thought of going 2 -> 3, and dont allow 1->3 or 3->2.

Anther thought:

a problem that comes up if we are taking an upgrade me approach instead of update to specific hash approach: The current method of upgrading (that is not documented at all) is through UpgradeSelf { hash } proposals. To keep everything simple for the user and also to easily track the upgrade history for a certain DAO, we should keep using the same proposal, but change the underlying implementation (this is what I've tried to achieve in this PR). So if the current state of art is that the proposal receives an hash and it upgrades to that hash, we should keep that logic, but maybe enforce a one step at a time approach on the UI side?

TC: I see what you're saying, I guess since it hasn't been established yet as a solid standard, this is the 1 chance to change it or accept it. IF the proposal has to supply a hash, it will require a method for factory to help identify next hash, at which point the user could submit invalid data on purpose or otherwise. It would fail on factory side, but would be valuable lost time & fee. IF the proposal is generic UpgradeSelf, then the UI / Users can be out of the failure loop. We could still issue a helper view method to let client UIs show the next hash, so DAOs can confirm, but it would not require extra UX to create proposals.

Decisions needed:

  1. Can we agree upon the version? I'm suggesting a u8 or similar, but not String
  2. Can we agree upon how to handle the UpgradeSelf proposal? I'd like your feedback @ctindogaru on above statement before making a decision.
ctindogaru commented 2 years ago

UnorderedMap<AccountId, u8>

I hear you, but I find it very hard to keep everything in sync in the factory. I don't know how the migration would look like.

From v2->v3, the DAO does not use the factory to migrate. So if a DAO migrated through UpgradeSelf { hash } and it's now v3, when you call the factory to check for the DAO version, the factory will tell you that the DAO is v2. This is because the factory has no way of knowing that the DAO did the upgrade on its own...

So you can't rely on the factory to tell you the right versions, because the upgrades from v2 to v3 will happen outside of the factory.

Can we agree upon the version? I'm suggesting a u8 or similar, but not String

u8 is perfect in my opinion and it's exactly what we need

Can we agree upon how to handle the UpgradeSelf proposal? I'd like your feedback @ctindogaru on above statement before making a decision.

You cannot remove the hash from UpgradeSelf, because in order for any v2 DAO to upgrade, the current proposal type requires a hash. So even if we are changing the implementation, you still need to go through UpgradeSelf { hash } from v2 to v3. And if we decide to do things differently in v3, we need to introduce a new type of proposal (UpgradeMe maybe?), because using UpgradeSelf without a hash is not backwards compatible.

ctindogaru commented 2 years ago

Can we agree upon the version? I'm suggesting a u8 or similar, but not String

Thinking better about this, I have fixed mixed feeling about it. u8 will limit you to a maximum of 256 versions. String provides more flexibility in that term + it allows you to specify a minor version of you want to.

Maybe you've fixed a bug that it's very urgent and you need to push it into production. You would call that version "v3.1" instead of "v4" because it's nothing major and it hasn't been audited. But you're sure it's pretty safe to push to production and it will fix a really ugly issue. I really like this kind of flexibility that you don't have with an integer.

ctindogarus4f commented 2 years ago

@TrevorJTClarke this is ready for review now

TrevorJTClarke commented 2 years ago

Noting discussion with @ctindogaru on version & UpgradeSelf:

  1. Version will be implemented as [u8; 2] for simple semver
  2. UpgradeSelf needs to continue including "hash" until it is possible for contracts to read code_hash for automatic upgrade checks.