[WIP] Conditions Hierarchy #444
Conversation
derekpierre
requested review from
Copilot,
cygnusv and
derekpierre
and removed request for
Copilot and
derekpierre
There was a problem hiding this comment.
Pull request overview
This is a Work-In-Progress (WIP) PR that introduces a comprehensive signing infrastructure called "Conditions Hierarchy" for the TACo network. The PR adds support for threshold signing multisig wallets that can be deployed across multiple chains, enabling cohort-based signature collection with cross-chain capabilities.
Key Changes
- New Signing Coordinator Infrastructure: Introduces
SigningCoordinator,SigningCoordinatorChild, andSigningCoordinatorDispatchercontracts to manage signing cohorts with cross-chain deployment support - Threshold Signing Multisig: Implements ERC-1271 compliant threshold multisig wallet with clone factory pattern for efficient deployment
- Cross-Chain Bridge Support: Adds Optimism-based L1/L2 bridge contracts (
OpL1Sender,OpL2Receiver) for cross-chain communication - Testing & Scripts: Comprehensive test coverage for new contracts and deployment/management scripts for lynx testnet
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/contracts/coordination/SigningCoordinator.sol | Main coordinator contract for managing signing cohorts with multi-chain support |
| contracts/contracts/coordination/ThresholdSigningMultisig.sol | ERC-1271 threshold multisig implementation with signer management |
| contracts/contracts/coordination/ThresholdSigningMultisigCloneFactory.sol | Factory for deterministic multisig clone deployment |
| contracts/contracts/coordination/SigningCoordinatorChild.sol | Child contract for deploying multisigs on target chains |
| contracts/contracts/coordination/SigningCoordinatorDispatcher.sol | Dispatcher for routing calls to appropriate chains |
| contracts/contracts/coordination/OpL1Sender.sol | L1 sender for Optimism bridge communication |
| contracts/contracts/coordination/OpL2Receiver.sol | L2 receiver for processing bridge messages |
| contracts/xchain/PolygonChild.sol | Added payable modifier to fallback function |
| tests/test_threshold_signing_multisig.py | Comprehensive tests for multisig functionality |
| tests/test_signing_coordinator.py | Tests for signing coordinator and cross-chain operations |
| scripts/form_signing_cohort.py | Script to initiate new signing cohorts |
| scripts/set_signing_cohort_conditions.py | Script to set conditions for cohorts |
| scripts/signing_cohort_additional_chain.py | Script to deploy cohort to additional chains |
| scripts/verify.py | Enhanced to support multiple contract verification |
| scripts/lynx/*.py | Deployment scripts for lynx testnet infrastructure |
| src/index.ts | Added SigningCoordinator to contract registry |
| test/registry.spec.ts | Updated tests for SigningCoordinator registry |
| requirements.txt | Updated Python dependencies |
| ape-config.yaml | Increased test account count to 70 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| require( | ||
| signingCohort.authority == msg.sender || serverAdmin.active, | ||
| signingCohort.authority == msg.sender || server.activeServerAdmins[msg.sender], |
There was a problem hiding this comment.
Does this mean that the cohort authority can override a server admin for setting the serverId-specific condition? If so, I don't think we want to give the cohort authority that power.
Only the server admin can set the serverId-specific condition. (cc @arjunhassard )
There was a problem hiding this comment.
But cohort admin can set itself to server admin anyway, right?
There was a problem hiding this comment.
You're right that technically they can take that route to set a serverid-specific condition.
Typically the cohort admin and server admins should be separate. They may be the same in Collab.Land's Discord server, but shouldn't be for other Discord servers.
| emit SigningCohortConditionsSet(cohortId, msg.sender, chainId, conditions); | ||
| } | ||
|
|
||
| function getUnsignedTransactionHash( |
There was a problem hiding this comment.
"If" we go this route (of getting conditions signed by the cohort), would a similar function to getSigningCohortDataHash(...) (which we use for the ritual) be simpler? I'm particularly worried about the use of nonce and that multisig function. wdyt?
There was a problem hiding this comment.
yeah, it can be without nonce similar to getSigningCohortDataHash, but it will open ability to execute same tx twice which in that case should not be big deal
| uint256[20] gap; | ||
| } | ||
|
|
||
| struct Server { |
There was a problem hiding this comment.
I think we should find a more generic term for Server, which is too specific to a particular use case. What about something like Domain?
|
|
||
| require( | ||
| signingCohort.authority == msg.sender || serverAdmin.active, | ||
| signingCohort.authority == msg.sender || server.activeServerAdmins[msg.sender], |
There was a problem hiding this comment.
But cohort admin can set itself to server admin anyway, right?
| ) external { | ||
| SigningCohort storage signingCohort = signingCohorts[cohortId]; | ||
| conditionsCheck(signingCohort, chainId); | ||
| require(serverId > 0, "Server id can't be zero"); |
There was a problem hiding this comment.
some optimization for getSigningCohortConditions
3 participants
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: