search

windranger-io / windranger-governance

BitDAO Governance contracts framework
Apache License 2.0
20 stars 10 forks source link

[Feature] Governor Alpha #76

Closed bitparadigm closed 2 years ago

bitparadigm commented 3 years ago

Closes https://github.com/windranger-io/windranger-governance/issues/75.

CjHare commented 2 years ago

question: just to be clear, is this a direct copy & paste of the Compund GovenorAlpha Solidity code?

The +1 on the last comment could either mean yes, or yes. I agree with the question being asked.

I could locally grab the Compund code and this PR and run a diff, but asking seemed easier. 🤔

bitparadigm commented 2 years ago

question: just to be clear, is this a direct copy & paste of the Compund GovenorAlpha Solidity code?

The +1 on the last comment could either mean yes, or yes. I agree with the question being asked.

I could locally grab the Compund code and this PR and run a diff, but asking seemed easier. 🤔

Almost, I had to still make very small tweaks to update from 0.5.11 which is Compound GovernorAlpha version to 0.7.6, but they are very minor, I don't think that will cause any security vulnerability as 0.7.6 was tested a lot compared to 0.8.x. You can run a diff and give your opinion.

CjHare commented 2 years ago

update from 0.5.11 which is Compound GovernorAlpha version to 0.7.6

What was the motivation to bump the version to the 0.7.x stream?

The version would benefit from bumping anyway if kept with 0.5.x (recommended being 0.5.17), but I'm interested to know the reasoning.

bitparadigm commented 2 years ago

update from 0.5.11 which is Compound GovernorAlpha version to 0.7.6

What was the motivation to bump the version to the 0.7.x stream?

The version would benefit from bumping anyway if kept with 0.5.x (recommended being 0.5.17), but I'm interested to know the reasoning.

0.5.11 is very outdated no?

CjHare commented 2 years ago

0.5.11 is very outdated no?

The 0.5.x stream is still in the recommended / acceptable range of the static analysers (specifically 0.5.16 - 0.5.17 is okay)

bitparadigm commented 2 years ago

0.5.11 is very outdated no?

The 0.5.x stream is still in the recommended / acceptable range of the static analysers (specifically 0.5.16 - 0.5.17 is okay)

I can use 0.5.17 for base GovernorAlpha. Though the only minor tweak that I will still need. I will paste Compound SafeMath add function inside of Timelock contract, instead of relying as a library on it because in https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L3 they do (I don't want to copy-paste their whole SafeMath into repo).

bitparadigm commented 2 years ago

Hmm, native version is 0.5.16 version and its recommended by Slither, my bad. I'm okay sticking with this version for GovernorAlpha if @shahankhatch is too. For all other contracts (experimental or adjusted forks) that we are making significant changes from our side we will stick with 0.8.x.

shahankhatch commented 2 years ago

Thanks for raising your points @CjHare. I'm okay with the suggested approach to use 0.5.16 while others use 0.8.x for our changes. This seems to me that it'll be a good balance from safe to iterating on the latest version to then take through an audit.

CjHare commented 2 years ago

This seems to me that it'll be a good balance from safe to iterating on the latest version to then take through an audit.

Indeed 👍