fix tokenvoting ivotes checks

[novaknole]

Description

Why this PR ?

This PR addresses the following problem: some users, even though they were using OZ's ERC20Votes, they didn't have ERC165(supportsInterface function) implemented in their contracts. This caused the problem where our TokenVotingSetup always would wrap such a token and the token applied on TokenVoting would be a wrapped one even though there was no need for that. Note that even though token would be wrapped, that wouldn't be a critical issue or cause any bricking because people would just call approve + depositFor and still maintain the same features. The problem I'm referring is that they would need to have extra layer of calling approve/depositFor which is not ideal and not only that, they were confused why the wrapping was necessary.

Does the solution solve it ?

There're 2 ways to solve this problem.

• Either remove the interface checks completely and don't do any other checks - this means that UI has to be doing the checks and TokenVotingSetup would just have the new boolean argument that would say whether user would want to wrap it or not. This flag(whether be true/false) would be figured out by UI checks.
• We still have the checks inside the contract but not the same ones. I removed the interface checks, but added a different type of checks that doesn't depend on ERC165 at all. Note that this is the solution what the PR is implemented with. It's also worth emphasizing that UI still also should be checking/using the same checks as I do in this PR to give users warning. i.e in other words, UI must remove supportsInterface check and exactly do the checks I have in this PR.

How the checks work:

// token = address(0) => deploy governanceERC20
// token != address(0)
//    token == IVote => do nothing and use this token directly
//    token != IVote => we wrap

---

// If token is IVote, it's either wrapped or not. We don't care which one is it because it already supports "getPastVotes, getVotes, getPastTotalSupply" functions.
// If token is not IVote, that means those 3 functions together are not supported by token. So we must wrap.

---

// If the token only has 2 out of those 3 functions, we would wrap. It might be the case that some contracts don't use all 
// these 3 functions, but only 2 of them, but problem why we would still need to wrap the token is that `TokenVoting` on 
// which token will be applied to, uses all these 3 functions. If we don't wrap, token will be applied but 
// TokenVoting will not work correctly since when it calls the function(not present in token), it will always fail.  This is why 
// we would always wrap unless all these 3 functions are present on the token - ("getPastVotes, getVotes, 
// getPastTotalSupply"). The scenario 
// when 2 out of 3 or 1 out of 3 are only present in token would happen if token deployer has no idea what he is doing or 
// doesn't follow OZ standards at all. In either case, UI will detect if not all 3 is present and if so, will give a warning to the 
// user that if he still submits this tx, his token will be wrapped which user will be confused about.

Why making this PR to main branch ?

If we want to have this fix before 1.4.0 is audited, then we can't use TokenVotingPlugin repo(post-split) since this repo uses 1.4.0. So we have no choice but to use main branch on osx. It would be great though NOT to merge this PR, because if we do, main branch will contain this new fix code whereas in audit report(by hellborn) didn't sign on this. It's a question of whether we want to have the non-audited code on main branch. I'd say no and go for the option that we leave this code on this PR, audit internally and anyways release it.

Can Old tokens that were wrapped incorrectly be fixed ?

The only fix is to add update code inside TokenVotingSetup that calls initializeV2 on the tokenVoting and passes new/fixed token. This has so many problems as if the proposal is already created on TokenVoting with old token, it must continue using old one and new proposals must use new tokens, but still, tokens that were wrapped incorrectly already use the funds on it and if we deploy new tokens, those funds won't be on this token. Long story short, this needs much,much deeper thoughts.

One more change in prepareUninstallation

We think that there's no need to revoke MINT_PERMISSION on the token contract for dao. Why ? Even though plugin(tokenvoting) was uninstalled, token still should continue to be operated by dao as it kind of belongs to dao. Think about it - if we still revoke this permission, nobody will have MINT_PERMISSION on the token and token becomes useless(only in minting case) even though it has funds ? We still think dao still should continue operating it. - Please think about this as we're not sold on either ideas.

Last thoughts

I haven't implemented the tests as I would first like to get a grasp of what you all thinking.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Giorgi Lagidze seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[brickpop]

Removing the mint permission would technically be a breaking change. It can still be granted later, though

[novaknole]

Why would it be breaking ? we only remove mint permission inside uninstallation of plugin.

[brickpop]

I see. The point being that it would be "breaking" in the sense of version 1 having one behaviour and version 2 having a different behaviour.

Not just adding a feature or fixing a bug, but instead changing the specs of what happens

[novaknole]

This change is not breaking in any sense at all. Nowhere in the UI, we say that we use the specific spec - This would be breaking in some sense if something was working before and now, it's not working the same way which is not the case here.