TACo Threshold Application #99

KPrasch · 2023-07-12T13:11:21Z

This PR introduces the contracts for installation of CBD+PRE as a threshold network application (Includes "Full PRE Application").

cygnusv · 2023-07-18T08:35:08Z

tests/application/conftest.py

+def encode_function_data(initializer=None, *args):
+    """Encodes the function call so we can work with an initializer.
+    Args:
+        initializer ([brownie.network.contract.ContractTx], optional):


cygnusv · 2023-07-18T08:37:06Z

contracts/contracts/PREApplication.sol

+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin-upgradeable/contracts/access/OwnableUpgradeable.sol";
+import "../threshold/IApplication.sol";
+import "../threshold/IStaking.sol";


I'm wondering if we could import these interfaces directly from the Threshold repo. See https://docs.apeworx.io/ape/stable/userguides/dependencies.html#github

Not necessarily a change we want to introduce now, but worth exploring

definitely, I planned do that (or at least try to) but forgot

cygnusv · 2023-07-18T09:19:34Z

contracts/contracts/PREApplication.sol

+
+        StakingProviderInfo storage info = stakingProviderInfo[_stakingProvider];
+        require(info.tReward > 0, "No reward to withdraw");
+        uint96 value = info.tReward;


Isn't it slightly more efficient to do the require after this line using value?

cygnusv · 2023-07-18T09:33:08Z

contracts/contracts/PREApplication.sol

+        if (info.authorized == 0) {
+            _stakingProviderFromOperator[info.operator] = address(0);
+            info.operator = address(0);
+            info.operatorConfirmed == false;


Do you mean = here? Same in line 457 and 483. Although, before that, see my comment in confirmOperatorAddress()

yeah, I fixed this bug in #98, and added proper tests

cygnusv · 2023-07-18T09:41:18Z

contracts/contracts/PREApplication.sol

+    /**
+    * @notice Make a confirmation by operator
+    */
+    function confirmOperatorAddress() external {


Let's take a step back here. Can we work without the operator confirmation? Making the operator node transact in both Ethereum mainnet (even if it's only for 1 TX) and Polygon (for rituals in the Coordinator) seems a hurdle for operators.

in that case I'd do confirmation on Polygon but it increases complexity to do that properly
and I'm not in favor of removing it, same reason as before: helps to exclude people who does nothing

helps to exclude people who does nothing

Given that for the moment we will be using the Merkle rewards approach, the operator confirmation (regardless if it's on mainnet or Polygon) is something we can drop. But let's leave this for further discussion, and not this PR.

…yers

… to set in app, restricts access to update PolygonRoot

…s bug with releasing operator

…perator with StakeInfo

App <-> bridge

derekpierre

🎸 Skimmed the tests but looks good.

contracts/contracts/Adjudicator.sol

scripts/deploy_taco_application.py

manumonti

LGTM!

I found some typos and TODO's which I suspect are easy to solve. But nothing relevant. Great job!

contracts/contracts/TACoApplication.sol

cygnusv · 2023-07-26T10:49:44Z

contracts/contracts/Adjudicator.sol


    mapping (address => uint256) public penaltyHistory;
    mapping (bytes32 => bool) public evaluatedCFrags;

+    uint256[50] private reservedSlots;


Weren't we going to remove it and re-introduce it if we make Adjudicator upgradeable? For reference: #100

cygnusv · 2023-07-26T10:50:11Z

contracts/contracts/SimplePREApplication.sol

-* @title PRE Application
-* @notice Contract handles PRE configuration
-*/
-contract SimplePREApplication {


cygnusv · 2023-07-26T11:02:46Z

contracts/contracts/TACoApplication.sol

+    /**
+    * @notice Make a confirmation by operator
+    */
+    function confirmOperatorAddress() external {


Not for this PR, but I'd get rid of operator confirmation. See #103

cygnusv · 2023-07-26T11:05:01Z

contracts/contracts/TACoApplication.sol

+    * @param _penalty Penalty
+    * @param _investigator Investigator
+    */
+    function slash(


Let's discuss (not for this PR) if we should not launch with slashing (or alternatively, deactivate slashing by tuning the parameters). See #104

cygnusv · 2023-07-26T11:08:09Z

contracts/contracts/coordination/IUpdatableStakeInfo.sol

@@ -6,7 +6,7 @@ pragma solidity ^0.8.0;
 * @title StakeInfo
 * @notice StakeInfo


We should change the docstrings here

cygnusv · 2023-07-26T11:15:12Z

scripts/deploy_taco_application.py


-    simple_pre = project.SimplePREApplication.deploy(
+    # TODO deploy proxy


Opened an issue to track this: #105

cygnusv · 2023-07-26T11:16:33Z

contracts/contracts/TACoApplication.sol

+    /**
+    * @notice Bond operator
+    * @param _stakingProvider Staking provider address
+    * @param _operator Operator address. Must be a EOA, not a contract address


Suggested change

* @param _operator Operator address. Must be a EOA, not a contract address

* @param _operator Operator address. Must be an EOA, not a contract address

Co-authored-by: Manuel Montenegro <manuel@nucypher.com> Co-authored-by: David Núñez <david@nucypher.com> Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

KPrasch assigned vzotova Jul 12, 2023

Full PRE application

f0ec623

vzotova force-pushed the threshold-app branch from e3a6358 to f0ec623 Compare July 14, 2023 21:06

vzotova changed the base branch from main to development July 14, 2023 21:06

cygnusv reviewed Jul 18, 2023

View reviewed changes

vzotova and others added 10 commits July 23, 2023 18:04

Address PR comments

7ef5d40

PolygonRoot: use StakeInfo interface to unify access regardless of la…

44fef54

…yers

Renames PREApplication to PRECBDApplication, adds bridge as parameter…

5ac78e5

… to set in app, restricts access to update PolygonRoot

Extracts Adjudicator from application to decrease contract size, fixe…

e71fbdd

…s bug with releasing operator

PRECBDApplication: syncing amount with StakeInfo, tests for syncing o…

176daed

…perator with StakeInfo

PRECBDApplication: tests for amount syncing with StakeInfo

8f83aeb

Renames PRECBD to TACo

40967b3

Apply suggestions from code review

e596bf1

Fix after rebase

eacb9b5

Merge pull request #98 from vzotova/cbd-pre-app

374af2c

App <-> bridge

vzotova changed the title ~~[WIP] TACo Threshold Application~~ TACo Threshold Application Jul 24, 2023

vzotova marked this pull request as ready for review July 24, 2023 14:10

vzotova requested review from arjunhassard, cygnusv, theref, derekpierre and manumonti July 24, 2023 14:11

derekpierre approved these changes Jul 24, 2023

View reviewed changes

contracts/contracts/Adjudicator.sol Outdated Show resolved Hide resolved

scripts/deploy_taco_application.py Outdated Show resolved Hide resolved

scripts/deploy_taco_application.py Outdated Show resolved Hide resolved

theref approved these changes Jul 25, 2023

View reviewed changes

manumonti reviewed Jul 25, 2023

View reviewed changes

This was referenced Jul 26, 2023

Reconsider operator confirmation for TACO app #103

Closed

Upgradeability tooling #105

Closed

cygnusv approved these changes Jul 26, 2023

View reviewed changes

cygnusv reviewed Jul 26, 2023

View reviewed changes

vzotova force-pushed the threshold-app branch from a20f9ba to b2a2428 Compare July 26, 2023 11:20

Apply suggestions from code review

3759907

Co-authored-by: Manuel Montenegro <manuel@nucypher.com> Co-authored-by: David Núñez <david@nucypher.com> Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

vzotova force-pushed the threshold-app branch from b2a2428 to 3759907 Compare July 26, 2023 11:22

manumonti approved these changes Jul 26, 2023

View reviewed changes

cygnusv merged commit 5251e07 into development Aug 1, 2023
4 checks passed

cygnusv mentioned this pull request Aug 22, 2023

Threshold Network Application contract high-level decisions/design #64

Closed

derekpierre mentioned this pull request Sep 11, 2023

L1<->L2 state synchronization #69

Closed

KPrasch deleted the threshold-app branch September 15, 2023 14:21

derekpierre mentioned this pull request Sep 18, 2023

v7.0.0 Major feature freeze nucypher/nucypher#3200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TACo Threshold Application #99

TACo Threshold Application #99

KPrasch commented Jul 12, 2023 •

edited

Loading

cygnusv Jul 18, 2023

vzotova Jul 23, 2023

cygnusv Jul 18, 2023

vzotova Jul 23, 2023

vzotova Jul 23, 2023

cygnusv Jul 18, 2023

vzotova Jul 23, 2023

cygnusv Jul 18, 2023

vzotova Jul 18, 2023

cygnusv Jul 18, 2023

vzotova Jul 18, 2023

cygnusv Jul 18, 2023

derekpierre left a comment

manumonti left a comment

cygnusv Jul 26, 2023

vzotova Jul 26, 2023

cygnusv Jul 26, 2023

cygnusv Jul 26, 2023

cygnusv Jul 26, 2023

cygnusv Jul 26, 2023

vzotova Jul 26, 2023

cygnusv Jul 26, 2023

cygnusv Jul 26, 2023

vzotova Jul 26, 2023

		@@ -6,7 +6,7 @@ pragma solidity ^0.8.0;
		* @title StakeInfo
		* @notice StakeInfo


		simple_pre = project.SimplePREApplication.deploy(
		# TODO deploy proxy

	* @param _operator Operator address. Must be a EOA, not a contract address
	* @param _operator Operator address. Must be an EOA, not a contract address

TACo Threshold Application #99

TACo Threshold Application #99

Conversation

KPrasch commented Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

manumonti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch commented Jul 12, 2023 •

edited

Loading