Skip to content

refactor: migrate from custom bitmask to EnumerableSet architecture#218

Merged
xDarksome merged 10 commits into
feat/contractfrom
refactor/cluster-contract
Jun 18, 2025
Merged

refactor: migrate from custom bitmask to EnumerableSet architecture#218
xDarksome merged 10 commits into
feat/contractfrom
refactor/cluster-contract

Conversation

@rplusq

@rplusq rplusq commented Jun 10, 2025

Copy link
Copy Markdown
Collaborator

Description

This is a follow-up PR on top of @xDarksome #212. My goal was to keep Rust/WCN integration almost unchanged while modernising the Solidity internals.

What changed (one-liner each)

  • Membership: custom 256-slot bitmask → OpenZeppelin EnumerableSet.
  • Ownership: custom owner fields → Ownable2Step (safer hand-off).
  • NodeOperator struct: now includes maintenance boolean (replaces separate mapping).
  • Migration FSM: same API, but isPulling set in a single loop (gas –5 k/op).
  • On-chain maintenance toggle: setMaintenance(bool) emits MaintenanceToggled.
  • Custom errors replace string reverts.
  • Settings & version: every mutator bumps version for subgraph indexing.

Integration side of things

  • All public getters (getCurrentKeyspace, getMigrationStatus, …) unchanged.
  • Events names unchanged + new MaintenanceToggled (ignored by nodes until used).
  • Gas profile: startMigration(16 ops) ≈ 140 k, setMaintenance ≈ 20 k.
  • Migration-to-main-net plan (unchanged)
  • Backend points to new address, runs e2e tests.

@xDarksome : please confirm events + getters still fit alloy bindings. No single getView() now, so you might have to join with multiCall on the alloy side: https://alloy.rs/guides/multicall#multicall-with-alloy

All forge tests green; ready for merge once you’re happy.

Comment thread contracts/.gas-snapshot
@@ -0,0 +1,84 @@
ClusterTest:test_AbortMigration_BumpsVersion() (gas: 288212)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This gas cost is for test functions, can we somehow get the cost of the actual SC calls here? I did manage to get it by forge test --via-ir -vvv --gas-report in my PR

@rplusq rplusq Jun 10, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is just the snapshot, runing what you said gives:

╭----------------------------------+-----------------+--------+--------+----------+---------╮
| src/Cluster.sol:Cluster Contract | | | | | |
+===========================================================================================+
| Deployment Cost | Deployment Size | | | | |
|----------------------------------+-----------------+--------+--------+----------+---------|
| 4462765 | 22888 | | | | |
|----------------------------------+-----------------+--------+--------+----------+---------|
| | | | | | |
|----------------------------------+-----------------+--------+--------+----------+---------|
| Function Name | Min | Avg | Median | Max | # Calls |
|----------------------------------+-----------------+--------+--------+----------+---------|
| abortMigration | 24957 | 48135 | 64963 | 64963 | 7 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| acceptOwnership | 30573 | 30573 | 30573 | 30573 | 2 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| addNodeOperator | 25846 | 120157 | 156915 | 156915 | 14 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| completeMigration | 24514 | 38973 | 43472 | 45878 | 12 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getAllOperators | 4035 | 287480 | 18550 | 839856 | 3 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getCurrentKeyspace | 8584 | 296314 | 22354 | 714140 | 5 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getMaintenanceOperators | 34185 | 34185 | 34185 | 34185 | 1 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getMigrationStatus | 5955 | 5955 | 5955 | 5955 | 7 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getOperatorAt | 3387 | 5399 | 6405 | 6405 | 3 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getOperatorCount | 2772 | 2772 | 2772 | 2772 | 6 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| getPullingOperators | 4018 | 20160 | 27818 | 28645 | 3 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| info | 8419 | 8427 | 8428 | 8428 | 12 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| isInitialized | 2946 | 2946 | 2946 | 2946 | 2 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| isOperator | 3999 | 3999 | 3999 | 3999 | 11 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| keyspaceVersion | 2984 | 2984 | 2984 | 2984 | 10 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| owner | 3073 | 3073 | 3073 | 3073 | 3 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| removeNodeOperator | 25095 | 54586 | 68064 | 80895 | 11 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| setMaintenance | 24294 | 53814 | 51632 | 77455 | 11 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| settings | 3170 | 3170 | 3170 | 3170 | 1 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| startMigration | 26402 | 636068 | 345575 | 13587430 | 35 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| transferOwnership | 25033 | 37408 | 37462 | 49675 | 4 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| updateNodeOperator | 25553 | 57678 | 73738 | 73738 | 9 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| updateSettings | 24527 | 46209 | 54882 | 54882 | 7 |
|----------------------------------+-----------------+--------+--------+----------+---------|
| version | 2786 | 2786 | 2786 | 2786 | 10 |
╰----------------------------------+-----------------+--------+--------+----------+---------╯

Ran 1 test suite in 33.14ms (24.17ms CPU time): 95 tests passed, 0 failed, 0 skipped (95 total tests)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean, I think we should either have the actual gas cost in this file or just remove it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I checked the gas cost, looks good. However this file is not representative, can we remove it?

Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol Outdated
Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol Outdated

@xDarksome xDarksome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good overall, gas cost seems to be reasonable as well, thanks!

Couple of final logic tweaks left.

Comment thread contracts/src/Cluster.sol
Comment thread contracts/src/Cluster.sol
@rplusq rplusq requested a review from xDarksome June 18, 2025 10:34
@rplusq

rplusq commented Jun 18, 2025

Copy link
Copy Markdown
Collaborator Author

Addressed feedback, thanks again @xDarksome

@xDarksome

Copy link
Copy Markdown
Member

👍 merging

@xDarksome xDarksome merged commit 2c12dfa into feat/contract Jun 18, 2025
10 checks passed
@xDarksome xDarksome deleted the refactor/cluster-contract branch June 18, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants