Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

voting-contract: Redesign how we store/update proposal status #63

Open
uint opened this issue Feb 2, 2022 · 7 comments
Open

voting-contract: Redesign how we store/update proposal status #63

uint opened this issue Feb 2, 2022 · 7 comments
Labels
question Further information is requested

Comments

@uint
Copy link
Contributor

uint commented Feb 2, 2022

The whole thing with using current_status and update_status helpers has proven to be confusing and encourage bugs. We should probably sit down, think about all the things we want this to do and redesign it in a way that's easy to understand and difficult to make mistakes with.

Very loose ideas:

  • Make the actual internally stored status impossible to access directly, only provide status access through accessors.
  • Create two distinct types - one type for what we store (or a set of flags?) and one type for what we show to the user.
  • Don't store status at all - chances are it can be deduced from all the other data we already have (voting period, existing votes, rules, etc).
  • ???
@hashedone
Copy link
Collaborator

  1. Impossible to do. Any state stored in blockchain is accessible directly by design. Even from external contract. Even if you will obfuscate it, it is possible and someone would use it in invalid way somewhere in the future.
  2. Reasonable, I think good to think about this.
  3. I would love this as it is universally proper. There is only problem I see - we need to investigate if it is possible to do cheaply. Also note, that this approach disallows for raw querying for status, and smart queries are more expensive, but being serious - because of how it works (no way to keep state updated - in best case we can update it as side-step of other executions), state should actually be always calculated, so never raw-queried, so this is basically not such a problem.

I personally think that we should try to go with 3, and if it is not easy to do think if 2 is maybe good enough. And obviously - maybe someone would figure out best idea.

What is critical to understand is that there are cases when state changed, but it causes execution to fail (one tries to vote on proposal which timeline exceed) - we just cannot update it because we would return error from contract, so whole state change would be rolled back (so both 1 and 2 would not work - and this is because I opt for 3).

The open question is - do we need notion of non-transactional updates in our contracts (meaning: state is updated no matter if contract failed). I am trying to figure out if there is a way to overuse this. However it could be helpful in some cases (like figuring out that state is invalid and store info in BC so later it would be decided way earlier).

@uint
Copy link
Contributor Author

uint commented Feb 2, 2022

Side note: what's the point of having the close call at all? It sets the status internally to Rejected, but a query will show that proposal as Rejected even without closing.

@ueco-jb
Copy link

ueco-jb commented Feb 3, 2022

@uint proposal is Close when it's voted within expiration time and is ready to be executed.

@uint
Copy link
Contributor Author

uint commented Feb 3, 2022

@uint proposal is Close when it's voted within expiration time and is ready to be executed.

Do you mean the Passed status? I mean the close fn, which literally changes the proposal's status to Rejected.

@ueco-jb
Copy link

ueco-jb commented Feb 3, 2022

🤦‍♂️ Yeah, I mixed those, sorry for confusion.

I think close is needed as well. Isn't that the only function, that actually sets status Rejected? So that content of persistent storage actually maps proper status?

@ethanfrey ethanfrey added this to the 0.7.0 Dry Run Net milestone Feb 3, 2022
@ethanfrey ethanfrey added the question Further information is requested label Feb 3, 2022
@uint
Copy link
Contributor Author

uint commented Feb 3, 2022

I think close is needed as well. Isn't that the only function, that actually sets status Rejected? So that content of persistent storage actually maps proper status?

It is, but it doesn't seem like internal status being set to Rejected matters for anything. It's not used within contract logic, and from the perspective of the user it kind of goes like this:

  1. A proposal expires without enough YES votes.
  2. I query the proposal, see that it's Rejected.
  3. I execute close on the proposal to change internal status to Rejected.
  4. I query the proposal and still see that it's Rejected.

@ueco-jb
Copy link

ueco-jb commented Feb 3, 2022

In my opinion, having internal status matching actuall status is good for

  1. consistency
  2. as "measure" of preventing possible bugs, if for example we add some extra query and forget to use current_status() (and use status instead - it already happend, why not again?) etc.
  3. my own sanity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants