Operator confirmation from Coordinator to TACo application by vzotova · Pull Request #114 · nucypher/nucypher-contracts

vzotova · 2023-08-25T19:12:00Z

Based on #102
Fixes #112
Refs #69

Renames StakeInfo and related interfaces
Changes node confirmation: Coordinator.setPublicKey is the source of node confirmations

cygnusv · 2023-08-30T10:14:11Z

contracts/xchain/PolygonChild.sol


-contract PolygonChild is FxBaseChildTunnel, Ownable {
+contract PolygonChild is ITACoChildToRoot, FxBaseChildTunnel, Ownable {
    event MessageReceived(address indexed sender, bytes data);


This event was removed in a previous commit of my sync branch, since the bridge can't process events in the _processMessageFromRoot callback (see commit b2ea8d9 description)

See https://github.com/0xPolygon/fx-portal/blob/296ac8d41579f98d3a4dfb6d41737fae272a30ba/contracts/tunnel/FxBaseChildTunnel.sol#L58

For both Ethereum Mainnet - Polygon Mainnet and Ethereum Goerli - Polygon Mumbai. See https://wiki.polygon.technology/docs/pos/design/bridge/l1-l2-communication/state-transfer/#prerequisites

Instead of setting the child tunnel with setFxChildTunnel(), which could be prone to front-running.

This way they can grant/revoke updaters, which is useful for testnets, and the admin can revoke themselves if the updaters list is considered final.

Based on the FlatRateFeeModel script

…nnet

…ed contract, makes child app upgradeable,

Update the x chain deploy script for full TACO test

piotr-roslaniec · 2023-09-07T09:09:53Z

contracts/contracts/coordination/Coordinator.sol


        ParticipantKey memory newRecord = ParticipantKey(lastRitualId, _publicKey);
-        keysHistory[provider].push(newRecord);
+        // keysHistory[stakingProvider][-1].publicKey != _publicKey; // TODO it's a question


I'm in favor of adding this check and preventing unnecessary updates. Allowing this update could trigger some confusing behavior (emitting events, other contract calls, etc.). I think it also prevents confusion on the operator's part - Why would they make an unnecessary update? Could be an operator error.

agree, but coordinator updates will be in some future PR, here just to not forget

piotr-roslaniec · 2023-09-07T09:28:56Z

contracts/contracts/coordination/TACoChildApplication.sol

+    function setCoordinator(address _coordinator) external onlyRole(DEPLOYER_ROLE) {
+        require(coordinator == address(0), "Coordinator already set");
+        require(_coordinator != address(0), "Coordinator must be specified");
+        // require(_coordinator.numberOfRituals() >= 0, "Invalid coordinator");


I see this require is commented out, but I wanted to ask what would motivate this check. Does Coordinator need to be in a certain state (has at least one completed ritual) to be considered a valid Coordinator in the app? Why? Is there some data/state dependency? between the two?

similar checks in other places work as proof that provider contract has proper abi (or at least one method)

Should we uncomment it?

Uncommented and updated that check

piotr-roslaniec · 2023-09-07T09:33:51Z

contracts/contracts/coordination/TACoChildApplication.sol

+
+        if (amount != fromAmount) {
+            info.authorized = amount;
+            emit AuthorizationUpdated(stakingProvider, amount);


I noticed that we're emitting an event here. AuthorizationUpdated, but we're not emitting any events in confirmOperatorAddress. Whos is the consumer for the AuthorizationUpdated event?

thanks I'll add event OperatorConfirmed

apparently it was already added in that commit bef7fa1

piotr-roslaniec · 2023-09-07T09:37:03Z

contracts/contracts/coordination/TACoChildApplication.sol

-        }
        require(info.authorized > 0, "No stake associated with the operator");
+        // TODO maybe allow second confirmation, just do not send root call
+        require(!info.operatorConfirmed, "Can't confirm same operator twice");


IMHO I'd just throw, but I have a bias for throwing

Throw will allow to use same tx again, and I don't think it would be good. If tx succeeded then you can't use it twice in a bridge

derekpierre · 2023-09-06T16:06:36Z

ape-config.yaml

+        reward_duration: 604800 # one week in seconds
+        deauthorization_duration: 5184000 # 60 days in seconds
        verify: False
    rinkeby:


Do we still need rinkeby at all? Can we remove it?

derekpierre · 2023-09-06T16:18:46Z

contracts/contracts/coordination/TACoChildApplication.sol

+        address stakingProvider = stakingProviderFromOperator[_operator];
+        StakingProviderInfo storage info = stakingProviderInfo[stakingProvider];
+        require(info.authorized > 0, "No stake associated with the operator");
+        // TODO maybe allow second confirmation, just do not send root call?


Perhaps I don't get the complete picture - but this makes sense to me. If already confirmed, no need to notify root application as opposed to the require failing.

this question more to coordinator. If Coordinator will know when to do confirmation (once per bond) or not, more like for future

derekpierre · 2023-09-07T15:43:43Z

contracts/contracts/coordination/TACoChildApplication.sol

+    function setCoordinator(address _coordinator) external onlyRole(DEPLOYER_ROLE) {
+        require(coordinator == address(0), "Coordinator already set");
+        require(_coordinator != address(0), "Coordinator must be specified");
+        // require(_coordinator.numberOfRituals() >= 0, "Invalid coordinator");


Should we uncomment it?

derekpierre · 2023-09-07T15:46:29Z

contracts/xchain/PolygonRoot.sol

-    modifier onlySource() {
-        require(msg.sender == source, "Caller must be the source");
+    modifier onlyRootApplication() {
+        require(msg.sender == rootApplication, "Caller must be the root app");


derekpierre · 2023-09-07T15:53:54Z

scripts/deploy_flat_rate_fee_model.py

+@click.option("--currency", default=ZERO_ADDRESS)
+@click.option("--rate", default=0)


Does it make sense to have defaults here? Are these default values actually allowed?

…nements

cygnusv · 2023-09-08T06:29:23Z

contracts/contracts/coordination/TACoChildApplication.sol

+    }
+}
+
+contract TestnetTACoChildApplication is AccessControlUpgradeable, TACoChildApplication {


cygnusv · 2023-09-08T06:33:17Z

Merging this will also merge commits originally intended by #102 but that were incorrectly merged to the development branch (I didn't notice that PR was targeting development). No harm done anyway since this PR was always based on top of #102

vzotova self-assigned this Aug 25, 2023

cygnusv changed the base branch from main to development August 29, 2023 09:51

cygnusv changed the base branch from development to main August 29, 2023 09:51

cygnusv reviewed Aug 30, 2023

View reviewed changes

cygnusv and others added 10 commits August 30, 2023 13:48

FxBaseChildTunnel can't emit events on _processMessageFromRoot callback

0d05a3c

See https://github.com/0xPolygon/fx-portal/blob/296ac8d41579f98d3a4dfb6d41737fae272a30ba/contracts/tunnel/FxBaseChildTunnel.sol#L58

FxPortal Deployment info: FxRoot, FxChild, CheckpointManager

38870ed

For both Ethereum Mainnet - Polygon Mainnet and Ethereum Goerli - Polygon Mumbai. See https://wiki.polygon.technology/docs/pos/design/bridge/l1-l2-communication/state-transfer/#prerequisites

Set FxChildTunnel on PolygonRoot constructor

e54455b

Instead of setting the child tunnel with setFxChildTunnel(), which could be prone to front-running.

Grant DEFAULT_ADMIN_ROLE on StakeInfo to the deployer

f2953e7

This way they can grant/revoke updaters, which is useful for testnets, and the admin can revoke themselves if the updaters list is considered final.

Adapt xchain deployment script to new changes

9551852

Improve logging of check_xchain script

ff108c7

Fix bugs and improve config handling for deploying xchain scripts

1dfe46f

New coordinator deployment script

a0acacd

Based on the FlatRateFeeModel script

Rename StakeInfo and interfaces. Add confirmation from Polygon to Mai…

4edfe53

…nnet

Fixes tests after renaming and adding confirmation through Coordinator

cd5dc14

vzotova force-pushed the confirmation branch from 8d674b3 to 3afb6c9 Compare August 30, 2023 13:22

vzotova added 2 commits August 30, 2023 09:29

Removes batchUpdate, moves additional access in child app to extend…

c74edc6

…ed contract, makes child app upgradeable,

Update contracts for coordinator test

7b7899c

vzotova force-pushed the confirmation branch from 3afb6c9 to 7b7899c Compare August 30, 2023 13:30

theref and others added 3 commits August 30, 2023 17:30

Update the x chain deploy script for full TACO test

19ca893

Merge pull request #1 from theref/taco_x_chain

ef6c536

Update the x chain deploy script for full TACO test

Tests for child app

bef7fa1

vzotova force-pushed the confirmation branch 9 times, most recently from 7ce559f to 69b22bc Compare September 1, 2023 02:52

vzotova force-pushed the confirmation branch 4 times, most recently from d7c6265 to 6e91874 Compare September 1, 2023 03:05

Updates dependencies and config to use deploy_xchain_test

5bce831

vzotova force-pushed the confirmation branch from 6e91874 to 5bce831 Compare September 1, 2023 20:51

vzotova marked this pull request as ready for review September 1, 2023 20:51

Final renames StakeInfo -> TACoChildApp

ff36f5b

vzotova changed the title ~~[WIP] Operator confirmation from Coordinator to TACo application~~ Operator confirmation from Coordinator to TACo application Sep 1, 2023

vzotova requested review from KPrasch, arjunhassard, cygnusv, derekpierre, jMyles, manumonti and theref September 1, 2023 21:10

vzotova mentioned this pull request Sep 5, 2023

TACo commitment #115

Merged

Checks X chanin txs in bridge contracts

34089fa

piotr-roslaniec reviewed Sep 7, 2023

View reviewed changes

piotr-roslaniec approved these changes Sep 7, 2023

View reviewed changes

derekpierre reviewed Sep 7, 2023

View reviewed changes

Changes functions in polygon bridge to fallback functions, minor refi…

b2efc21

…nements

cygnusv approved these changes Sep 8, 2023

View reviewed changes

cygnusv merged commit 3456fdc into nucypher:main Sep 8, 2023

cygnusv mentioned this pull request Sep 8, 2023

Improvements to state synchronization bridge #102

Merged

This was referenced Sep 12, 2023

Coordinator Alpha 13 Support nucypher/nucypher#3227

Merged

v7.0.0 Major feature freeze nucypher/nucypher#3200

Closed

vzotova deleted the confirmation branch January 28, 2025 18:19

		@click.option("--currency", default=ZERO_ADDRESS)
		@click.option("--rate", default=0)

Conversation

vzotova commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cygnusv commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vzotova commented Aug 25, 2023 •

edited

Loading