Staking contract fix

[sparrowDom]

It says in the ERC20Votes documentation that the delegate function needs to be called in order for the votes and voting checkpoints to register. Without this change the Governor's _getVotes that calls token's getPastVotes just returns 0 all the time. This has been introduced as a gas saving strategy.

In case we go with this solution we would probably need to:

- re-deploy the staking contract with our own overriden implemetation of Erch20Votes that allows the governor to backfill the self-delegation / checkpoint data - backfill all the data of existing stakes

Note: tests didn't fail because we were self-delegating in them: https://github.com/OriginProtocol/ousd-governance/blob/master/tests/governance/test_vote.py#L16~~

Final solution: have client check if the account has performed delegation on staking contract when: creating proposal / casting a vote. If not perform self delegation before the action

[DanielVF]

Thanks for tracking this down!

Most people won't vote - only a tiny percentage. I think it's fine to keep the contract simpler and cheaper.

Don't we just need to have UI way to delegate your vote? And check that users have delegated votes before letting them vote/propose?

[sparrowDom]

Great point @DanielVF. Have implemented the suggested fix in the client. This is ready to be reviewed again.

I have also fix the issue in the contract. So when we re-deploy the staking contract the client won't need to perform the double transaction. You ok with keeping the fix in the contract as well?

[sparrowDom]

hmm good point that would make the UI more consistent. And there is a legitimate concern that users might be worried what are the 2 transactions about. Quickly mocking up how the UI with 2 buttons might look like... From this:

to this:

and (maybe?) from this:

to this:

I like the how the proposal creation part looks much more than the voting part. Additional thoughts on it:

• the UI with disabled button would feel more natural to me if was only part of 1 flow. In effect issuing delegation on proposal creation or voting will show it disabled on both places. That could be confusing
• Delegate is probably a bad call to action. Something more along the lines of "Register as voter" or something similar would probably make more sense to the end user.
• I lean towards not seeing much added value to disabled (account has already delegated) button after the first proposal creation / voting flow.

I would favour one of the below solutions - happy to be convinced to the contrary:

• leave the flow as it is
• make the "Register to vote" flow as a modal that pops up only first time for the user. Where there is small text explaining that this registration only happens once for each account. Along with the button issuing delegation. When transaction is mined the modal disappears and is never shown again.

@tomhirst @micahalcorn your thoughts?

[tomhirst]

@sparrowDom I think the side-by-side pattern works well - though we should use the same button styling as we do on the staking experience:

The proposal creation view needs work, but the proposal single view needs more. It would be great to gather some examples of other governance portals so we can see how they present their votes. Compound was mentioned in our last discussion so I can start there once we get this PR merged.

On "Delegate". Yeah, I don't like it either because it sounds confusing. We have a modal component that the user will be used to throughout the dApp, so I'm in favour of adding one-off delegation education/transaction initiation using that pattern.

I guess this is all null and void if we merge your suggested contract change as the delegation will happen within the same transaction should it be required (the first proposal submission), right?

[sparrowDom]

Yeah this will be null for the new stakers. The ones who have already claimed rewards / have existing stakes will still need the modal. The modal of course won't show to the new stakers since their delegation call would have already been made.

[sparrowDom]

Might need some more UI treatment (cc @micahalcorn ) but this is the flow now:

I am pretty happy with the functionality / flow. At least the success modal screen probably needs some more UI treatment. @tomhirst you happy with this and we can merge, or you think we should do some more tweaks?

[tomhirst]

Might need some more UI treatment (cc @micahalcorn ) but this is the flow now:

I am pretty happy with the functionality / flow. At least the success modal screen probably needs some more UI treatment. @tomhirst you happy with this and we can merge, or you think we should do some more tweaks?

Nice one @sparrowDom! I have just tested this as (mostly) working as intended and managed to submit a proposal locally.

Just left a couple of comments, otherwise, I am fine with the UI/UX as a v1.

1 important point:

We shouldn't merge this to master (or at least merge to stable) until we have an environment flag in place that enables (or at least shows) governance features. That way we have more control over when the features go out publicly while we still work on them within our usual development flow.

Could we get that into this PR? When this flag is active we need (bare minimum):

• To change the redirect of / to /claim
• Show the overview page at /
• Add the Overview and Proposals links to the main nav

[sparrowDom]

Thanks @tomhirst! Sloppy of me to let the mistake slip.

I've spent waaay to long on this PR, I blame it on post vacation brain laziness :D There were 2 problems why the confirmation modal was showing up too much. One was me not properly propagating states. And the other more interesting one, is that:

• previous code has created a delegation call
• because when signing transactions that uses a Metamask provider
• and then the code immediately queried (refreshed account state) the contract for that delegation address using the default contract provider (Alchemy)
• and of course state takes some time to propagate on ethereum and is only eventually "correct"

Funny is this state propagation issue also surfaces when developing on local - where provider is the local eth node. Which is somewhat puzzling.... The takeaway here is that don't rely on state being updated in a short interval when reading from blockchain the data that has just been updated.

[sparrowDom]

This is ready for another review. Oh and also if OK with you @tomhirst for now have just hidden the Proposal navigation item. This PR is already getting pretty beefy, and wouldn't like to creep more functionality into it.

[tomhirst]

Nice one @sparrowDom and thanks for documenting the strange edge case.

This LGTM now.

Let's deal with the environment variable flag in another PR.

Merging 🚀