delvtech / council

Flexible DAO governance smart contracts, developed by Delv.
Apache License 2.0
93 stars 19 forks source link

Avoid invalid staleBlock #81

Closed ryangoree closed 1 year ago

ryangoree commented 1 year ago

While testing on Hardhat, the subtraction of staleBlockLag from block.number threw "Panic" errors since there weren't enough blocks mined yet.

This change would make it easier to deploy Council to a fresh chain like a local testnet.

We could alternatively add a require(_staleBlockLag <= block.number, "...") to the constructor, but then you'd need to adjust your deployment arguments depending on the network and settle for a very short stale block lag when it is a fresh chain. Not sure how to test the effect on gas cost 🤷

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4757948072


Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/vaults/VestingVault.sol 0 4 0.0%
<!-- Total: 4 8 50.0% -->
Totals Coverage Status
Change from base Build 4347624322: -0.7%
Covered Lines: 514
Relevant Lines: 534

💛 - Coveralls
ryangoree commented 1 year ago

I generally don't support merging code into our main contracts for the purpose of ease of testing convenience. In particular this extra code now lives in every vote action call. If we do want to make a change like this we should add at most a constructor check on the staleBlock lag vs the blocknumber.

Ok, yeah that's fair. I'll close this one.