search

Meridian-IE / impact-evaluator

Impact Evaluator smart contract
Other
4 stars 0 forks source link

refactor `Balances` contract #53

Closed juliangruber closed 1 year ago

juliangruber commented 1 year ago

Moving all the balances logic into a separate contract, for readability

bajtos commented 1 year ago

Is it worth adding some basic/smoke tests for this new Balances contract?

juliangruber commented 1 year ago

Let me know what you think. If you prefer, you can land this PR as-is and improve the design in subsequent PRs.

Perfect, will do in a separate PR, as this problem already exists in main.

Thank you so much for taking a closer look at the balances logic, we really need to get this part airtight

juliangruber commented 1 year ago

Is it worth adding some basic/smoke tests for this new Balances contract?

I don't see the need for that atm, I think it would be better to add more tests (for this logic) to the main contract test suite. Inheriting from another contract is already a security risk (in Solidity it's considered safer to repeat code and have everything in one file), I think we should make the distinction between the two as little as possible