-
Notifications
You must be signed in to change notification settings - Fork 12
[WIP] Conditions Hierarchy #444
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
base: signing
Are you sure you want to change the base?
Changes from 1 commit
ffcd98d
5c394bd
b61c1cc
c28b8f7
207e960
23fd883
0570525
64a4578
0fae651
f1afe8a
718a12c
546021f
918ef37
25d3135
9ad0d30
4254cf3
b3137f1
20df5b2
602bca1
e6a1927
15dc5db
2908cc0
d2d20d1
ddd4b54
0303b13
3f557d9
7941cef
3d0b324
589daf9
e701331
47b4379
c68f1de
ebe47c0
17087f6
20620d8
b2adee2
c8f5e63
92609e9
7360cd0
e6ac365
ec420dd
c0da4a3
40bc0b3
9901070
90bd925
fdbd793
6ca8925
55e5243
a1328a9
538c519
1c17598
2613b84
d2f8bbf
4106327
8299bd6
61424c1
b6b4765
8bed9da
9083b57
1c646e4
1afcab2
34bb21a
aac4cc2
7d65022
d3646d5
0248308
1bd49cd
e414d9c
c9d25d6
ab01b9d
8a088aa
0e58a0a
c7ab113
ac07cf8
08c176e
6eddbdd
205e7df
d315bf8
720cb9b
8ec51dc
3485c46
df34d71
54578a6
6a64789
23517ad
6762e0a
86983b3
2e7f5ca
ff57ccb
66fc2dd
466eac6
b0d56dd
39722b2
3285d85
5804859
98c3f91
7588451
406295c
48273ab
cba3b3a
5a3f9a9
84c0de8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,9 @@ import "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; | |
| import "@openzeppelin-upgradeable/contracts/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; | ||
| import "@openzeppelin-upgradeable/contracts/proxy/utils/Initializable.sol"; | ||
| import "./ISigningCoordinatorChild.sol"; | ||
| import "./SigningCoordinatorChild.sol"; | ||
| import "./SigningCoordinatorDispatcher.sol"; | ||
| import "./ThresholdSigningMultisig.sol"; | ||
| import "../TACoApplication.sol"; | ||
|
|
||
| // SigningCoordinator ----> Dispatcher ----> (Relevant) L1Sender ---------[BRIDGE]---------- L2Receiver ----> SigningCoordinatorChild (1. deploys multisig OR 2. updates multisig) | ||
|
|
@@ -310,11 +312,25 @@ contract SigningCoordinator is Initializable, AccessControlDefaultAdminRulesUpgr | |
| emit SigningCohortConditionsSet(cohortId, msg.sender, chainId, conditions); | ||
| } | ||
|
|
||
| function getUnsignedTransactionHash( | ||
| ThresholdSigningMultisig multisig, | ||
| address serverAdmin, | ||
| uint32 cohortId, | ||
| uint256 chainId, | ||
| bytes memory conditions, | ||
| uint256 serverId | ||
| ) public view returns (bytes32) { | ||
| bytes memory data = abi.encodePacked(cohortId, serverId, chainId, conditions); | ||
| uint256 nonce = multisig.nonce(); | ||
| return multisig.getUnsignedTransactionHash(serverAdmin, address(this), 0, data, nonce); | ||
| } | ||
|
|
||
| function setSigningCohortConditions( | ||
| uint32 cohortId, | ||
| uint256 chainId, | ||
| bytes calldata conditions, | ||
| uint256 serverId | ||
| uint256 serverId, | ||
| bytes calldata signature | ||
| ) external { | ||
| SigningCohort storage signingCohort = signingCohorts[cohortId]; | ||
| conditionsCheck(signingCohort, chainId); | ||
|
|
@@ -325,6 +341,26 @@ contract SigningCoordinator is Initializable, AccessControlDefaultAdminRulesUpgr | |
| signingCohort.authority == msg.sender || server.activeServerAdmins[msg.sender], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 )
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But cohort admin can set itself to server admin anyway, right?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed privilege for cohort admin |
||
| "Only the cohort authority or a server admin can set conditions" | ||
| ); | ||
|
|
||
| SigningCoordinatorChild child = SigningCoordinatorChild( | ||
| getSigningCoordinatorChild(block.chainid) | ||
| ); | ||
| ThresholdSigningMultisig multisig = ThresholdSigningMultisig( | ||
| child.cohortMultisigs(cohortId) | ||
| ); | ||
|
|
||
| bytes32 hash = getUnsignedTransactionHash( | ||
| multisig, | ||
| msg.sender, | ||
| cohortId, | ||
| chainId, | ||
| conditions, | ||
| serverId | ||
| ); | ||
| require( | ||
| multisig.isValidSignature(hash, signature) == multisig.MAGICVALUE(), | ||
| "Invalid Signature" | ||
| ); | ||
| server.conditions[chainId] = conditions; | ||
| emit SigningCohortConditionsSet(cohortId, msg.sender, chainId, conditions); // TODO log serverId | ||
| } | ||
|
|
@@ -473,7 +509,7 @@ contract SigningCoordinator is Initializable, AccessControlDefaultAdminRulesUpgr | |
| } | ||
| } | ||
|
|
||
| function getSigningCoordinatorChild(uint256 chainId) external view returns (address) { | ||
| function getSigningCoordinatorChild(uint256 chainId) public view returns (address) { | ||
| address child = signingCoordinatorDispatcher.getSigningCoordinatorChild(chainId); | ||
| return child; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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 ofnonceand that multisig function. wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 dealThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed nonce and simplified in general