Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump hardhat to 2.20 #1254

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Bump hardhat to 2.20 #1254

wants to merge 10 commits into from

Conversation

kronosapiens
Copy link
Member

@kronosapiens kronosapiens commented May 15, 2024

Main

  • Update to Hardhat 2.20 and cancun hardfork
  • Remove calls to selfdestruct and update uninstall behavior for extensions
  • Make better use of ColonyExtension.sol base class and remove boilerplate from extension contracts
  • Make ColonyExtension.resolver public to facilitate testing of new uninstall behavior

Misc

  • Miscellaneous improvements to coverage
  • Split up reputation-test-non-gnosis Circle job to reduce total build times
  • Make arguments to getMetatransactionNonce consistent among affected contracts

@kronosapiens kronosapiens marked this pull request as draft May 15, 2024 18:39
@kronosapiens kronosapiens force-pushed the maint/hardhat20 branch 3 times, most recently from 905c47b to 6181c43 Compare May 15, 2024 21:07
@kronosapiens kronosapiens marked this pull request as ready for review May 16, 2024 16:24
@kronosapiens kronosapiens force-pushed the maint/hardhat20 branch 3 times, most recently from 529bfe9 to 5271c41 Compare May 17, 2024 18:01
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make arguments to getMetatransactionNonce consistent among contracts

If this is the goal, then I believe you have missed:

  • ColonyNetworkAuction
  • IBasicMetaTransaction
  • CoinMachine
  • FundingQueue
  • TokenSupplier
  • Whitelist
  • VotingReputationStorage
  • VotingReputationMisaligned
  • TokenLocking

Did you not touch incrementMetatransactionNonce because it's internal, or some other reason?

Fun to see a refactor that reduces duplication to the extent the coverage thresholds fail so more tests have to be written, that's good to see 😄

.circleci/config.yml Show resolved Hide resolved
selfdestruct(colonyNetwork);
// Send ether to the metaColony
// slither-disable-next-line arbitrary-send-eth
payable(metaColonyAddress).transfer(address(this).balance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test that this is done (for all places where we do it, not just here). A good rule of thumb is that if you can remove a line of code and the tests still pass, you need to write more tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose. I would argue that we never tested the ether-returning function for selfdestruct, and so shouldn't need to test this now. But I can add an ether transfer check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but we did test that it self-destructed (and then trusted that the opcode did the rest of what it was meant to do). We could not include this functionality, but I think that's a much worse outcome.

@@ -34,7 +34,7 @@ abstract contract ColonyExtension is DSAuth, DSMath, PatriciaTreeProofs, Multica

event ExtensionInitialised();

address resolver; // Align storage with EtherRouter
address public resolver; // Align storage with EtherRouter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure I don't need to remind you of my distaste for changing code in order to accommodate the tests! This isn't necessary, as we can just instantiate a contract as an EtherRouter like we do in other tests:

const colonyNetworkAsEtherRouter = await EtherRouter.at(colonyNetwork.address);
const latestResolver = await colonyNetworkAsEtherRouter.resolver();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that earlier and it did not work, for the reason that extensions instantiated directly (like CoinMachine.new) don't in fact inherit from EtherRouter. Alternatives would be to add a getResolver view function in ColonyExtension.sol or to access the storage slot directly, which wouldn't be my preference.

contracts/extensions/ColonyExtension.sol Show resolved Hide resolved
docs/interfaces/extensions/coinmachine.md Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
@kronosapiens
Copy link
Member Author

kronosapiens commented May 22, 2024

Thinking about this more, I'm not sure if there's much benefit to including uninstall as part of the extension interface in general. We can still support uninstallExtension at the network-level, but the implementation should be changed to:

  1. Remove all permissions from the extension in the colony
  2. Delete the extension from the installations mapping

I say this because we already have a deprecate function which does all of the restricting that we would add to an uninstall function, which makes me think that an uninstall which locks all functions would be mostly redundant.

If anything, the uninstall function on the extension should perform checks to see if we are ready to uninstall the extension via the network. I'm thinking of streaming payments here, where uninstall would simply error if there were unclaimed funds.

@kronosapiens kronosapiens force-pushed the maint/hardhat20 branch 2 times, most recently from 53eae80 to 867a618 Compare May 30, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants