Core: GovernQueue test

[nivida]

closes #15

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ nivida
❌ novaknole1
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[nivida]

Note: I think we should/could move the GovernQueueStateLib into a separate file. This because the GovernQueuefile would look cleaner.

[nivida]

Note: The complexity of the GovernQueue test is from my point of view a clear sign for bad OOD. I think we should think again about the separation of concerns closer. This would increase the possibility to re-use code, easn the audit process, possibly lower the deployment cost of an ERC3k DAO in many cases, and improve the situation if single pieces of a DAO should get improved/changed.

[izqui]

I don't disagree, but also I wasn't optimizing for great OOD, nor I think we should.

Having a concise smart contract that can be read in one go without having to jump between multiple contracts is more important for me than easier testability. If a dev needs to change something, they only need to touch in one file without thinking about dependencies.

[nivida]

I don't disagree, but also I wasn't optimizing for great OOD, nor I think we should.

Having a concise smart contract that can be read in one go without having to jump between multiple contracts is more important for me than easier testability. If a dev needs to change something, they only need to touch in one file without thinking about dependencies.

Yes, I wouldn't do that just for better tests. But I thought there are probably also some other advantages.. 🤷‍♂️
@facuspagnuolo thought he will think about my feedback during his review :-)

[nivida]

Note: Tests can't have all the assertions they should have because of this bug TrueFiEng/Waffle#245

[nivida]

Use token contract from govern-token and add this package as devDependency

[nivida]

Move those test-helpers to the govern-contract-utils package instead of duplicating it. (Don't forget to update the test in the erc3k package)

[nivida]

... and add a unit-test test for those test-helpers cause of the complexity and importance they have.

[nivida]

Note: https://github.com/aragon/govern/blob/master/packages/govern-core/contracts/pipelines/GovernQueue.sol#L112

Does not collect the tokens as expected. It should instead of collecting and sending the token to the msg.sender send it to the Queue contract. This means the deposit collectFrom and releaseTo methods of the DepositLibneed to be changed accordingly (e.g.: safeTransferFrom should be used in releaseTo). Another possible solution would be to transfer the tokens to the ERC20 but I don't think this is something we really want to do :-)

[nivida]

Note: https://github.com/aragon/govern/blob/master/packages/erc3k/contracts/ERC3000Data.sol#L53

this should probably for reasons of syntactical correctness be explicitly transformed to an address with address(this)

[izqui]

We should finish this ASAP @nivida

[codecov-io]

Codecov Report

Merging #111 (05c196c) into master (93137b0) will increase coverage by 15.67%.
The diff coverage is 66.32%.

@@             Coverage Diff             @@
##           master     #111       +/-   ##
===========================================
+ Coverage   56.92%   72.59%   +15.67%     
===========================================
  Files          32       35        +3     
  Lines         527      624       +97     
  Branches       62       70        +8     
===========================================
+ Hits          300      453      +153     
+ Misses        227      171       -56     

Impacted Files	Coverage Δ
packages/govern-core/contracts/test/TestToken.sol	56.33% <56.33%> (ø)
...ages/govern-core/contracts/test/ArbitratorMock.sol	89.47% <89.47%> (ø)
...core/contracts/test/ArbitratorWrongSubjectMock.sol	100.00% <100.00%> (ø)
...govern-core/contracts/test/ERC3000ExecutorMock.sol	55.55% <100.00%> (+55.55%)	⬆️
...es/govern-core/contracts/pipelines/GovernQueue.sol	100.00% <0.00%> (+90.72%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93137b0...05c196c. Read the comment docs.

[nivida]

LGTM! Thanks for the finish @novaknole