Coordinator: moves rituals from array to mapping #307
Conversation
vzotova
changed the title
vzotova
requested review from
KPrasch,
cygnusv,
theref,
derekpierre and
manumonti
tests/test_coordinator.py
Outdated
| assert not ritual_struct[8] # aggregationMismatch | ||
| assert ritual_struct[9] == global_allow_list.address # accessController | ||
| assert ritual_struct[10] == (b"\x00" * 32, b"\x00" * 16) # publicKey | ||
| assert not ritual_struct[11] # aggregatedTranscript |
There was a problem hiding this comment.
If I'm understanding this test correctly, you are creating and checking 3 rituals:
- first one mimics ritual that was created before any concept of fee model - fee model is 0x...0
- second mimics ritual that was created with a fee model address but was initiated with the storage before your current storage change fix
- third mimics ritual that was created with your current storage change fix
For v7.5.x (so unreleased code for next release) we use index 12 for fee model, if available - nucypher/nucypher@2c2cb5d#diff-911a370c18d0d0e3ea62099fc5ff6daa16915da19cf5399f24e404b7d8d6d168R597.
- Is that still fine or will that be problematic?
- If not problematic, should we also check the values in the struct at index
12(fee model) in this test?
There was a problem hiding this comment.
for client that supports new fee model architecture - everything should stay the same, so yeah feeModel is 12th in structure.
I'll add test in check
There was a problem hiding this comment.
added this check
interesting that ritual_struct[12] is participants array and not fee model
I'm not sure if it's ape behavior or abi enhanced for nested structs comparing to what was before, anyway could you check if 7.5.x would work with this update, please
There was a problem hiding this comment.
interesting that ritual_struct[12] is participants array and not fee model
Indeed. This was how we realized we needed to get participants for a ritual in a separate call.
could you check if 7.5.x would work with this update, please
Ran the acceptance tests for v7.5.x with the following update to the ape-config.yaml:
diff --git a/tests/acceptance/ape-config.yaml b/tests/acceptance/ape-config.yaml
index b44995ae6..3d3d34ce2 100644
--- a/tests/acceptance/ape-config.yaml
+++ b/tests/acceptance/ape-config.yaml
@@ -5,8 +5,8 @@ plugins:
dependencies:
- name: nucypher-contracts
- github: nucypher/nucypher-contracts
- ref: main
+ github: vzotova/nucypher-contracts
+ ref: coordinator-layout
config_override:
solidity:
version: 0.8.23and the acceptance tests all pass without any changes 🎉 .
vzotova
force-pushed
the
coordinator-layout
branch
from
5ef57d7
to
2ee2d92
Compare
| @@ -128,35 +131,57 @@ contract Coordinator is Initializable, AccessControlDefaultAdminRulesUpgradeable | |||
| __AccessControlDefaultAdminRules_init(0, _admin); | |||
| } | |||
|
|
|||
| function initializeNumberOfRituals() external reinitializer(2) { | |||
There was a problem hiding this comment.
Should we include something to make sure that this function is called before the contract is used?
There was a problem hiding this comment.
I thought about that but honestly - it will only increase gas for checking and we can upgrade together with reinitialize (should be possible)
There was a problem hiding this comment.
Maybe let's add a dev comment reminding to use upgradeAndCall do it.
Co-authored-by: David Núñez <[email protected]>
|
Do we need a follow-up issue with a rollout plan for these changes? See #309 |
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: