Skip to content
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

Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child #3237

Merged
merged 24 commits into from
Sep 21, 2023

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre commented Sep 20, 2023

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

High-level idea of the changes introduced in this PR.
List relevant API changes (if any), as well as related PRs and issues.

Issues fixed/closed:

  • Fixes #...

Closes #3235 .

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #3237 (21aefb7) into development (0da1397) will increase coverage by 0.10%.
Report is 2 commits behind head on development.
The diff coverage is 87.09%.

@@               Coverage Diff               @@
##           development    #3237      +/-   ##
===============================================
+ Coverage        80.28%   80.38%   +0.10%     
===============================================
  Files              112      113       +1     
  Lines            11253    11300      +47     
===============================================
+ Hits              9034     9084      +50     
+ Misses            2219     2216       -3     
Flag Coverage Δ
acceptance 91.09% <100.00%> (+0.14%) ⬆️
integration 73.24% <78.94%> (+0.09%) ⬆️
unit 57.60% <47.36%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
nucypher/blockchain/eth/trackers/bonding.py 100.00% <ø> (ø)
nucypher/blockchain/eth/agents.py 55.51% <61.53%> (+0.45%) ⬆️
nucypher/types.py 85.71% <66.66%> (-14.29%) ⬇️
nucypher/blockchain/eth/actors.py 83.84% <95.83%> (+3.71%) ⬆️
nucypher/blockchain/eth/constants.py 100.00% <100.00%> (ø)
nucypher/blockchain/eth/utils.py 27.65% <100.00%> (+4.93%) ⬆️
tests/acceptance/actors/test_dkg_ritual.py 96.47% <100.00%> (+0.04%) ⬆️
tests/acceptance/actors/test_ursula_operator.py 93.10% <100.00%> (+1.10%) ⬆️
...s/acceptance/agents/test_taco_application_agent.py 100.00% <100.00%> (ø)
...ptance/agents/test_taco_child_application_agent.py 100.00% <100.00%> (ø)
... and 2 more

derekpierre added a commit to derekpierre/nucypher that referenced this pull request Sep 20, 2023
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Sep 20, 2023
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Sep 20, 2023
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Sep 21, 2023
@derekpierre derekpierre changed the title [WIP] TACo Application Child Agent to accommodate syncing issues between Root and Child TACo Application Child Agent to accommodate syncing issues between Root and Child Sep 21, 2023
@derekpierre derekpierre marked this pull request as ready for review September 21, 2023 01:02
@derekpierre derekpierre changed the title TACo Application Child Agent to accommodate syncing issues between Root and Child Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child Sep 21, 2023
@derekpierre derekpierre marked this pull request as draft September 21, 2023 01:11
@derekpierre derekpierre changed the title Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child [WIP] Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child Sep 21, 2023
else ""
)
emitter.message(
f"! Bonded staking provider address {truncate_checksum_address(taco_root_bonded_address)} on {taco_root_pretty_chain_name} not yet synced to child application on {taco_child_pretty_chain_name} {child_bonded_address_info}; waiting for sync",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this 🚀

@@ -10,6 +10,7 @@
STAKING_ESCROW_STUB_CONTRACT_NAME = "StakingEscrowStub"
ADJUDICATOR_CONTRACT_NAME = "Adjudicator"
TACO_APPLICATION_CONTRACT_NAME = "TACoApplication"
TACO_CHILD_APPLICATION_CONTRACT_NAME = "TACoChildApplication"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is looking good overall! I do have one comment related to this line: To avoid needing to change this line depending on whether or not a node is running on testnet vs mainnet, I assert that we always use this contract name in the registry, which is the historical convention anyways.

Copy link
Member Author

@derekpierre derekpierre Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I agree with that assertion. We'll need to modify some logic when deploying to lynx as part of the work for nucypher/nucypher-contracts#119

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose even for Lynx still will be TACoChildApplication abi, right?

Copy link
Member Author

@derekpierre derekpierre Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the ABI is the same, but we need to do some magic when generating the lynx registry since the contract names for root and child application contracts are different - https://github.com/derekpierre/nucypher-contracts/blob/lynx-deployment/contracts/contracts/testnet/LynxSet.sol.

@derekpierre derekpierre changed the title [WIP] Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child Use of TACoChildApplicationAgent for staking information and accommodate syncing issues between Root and Child Sep 21, 2023
@derekpierre derekpierre marked this pull request as ready for review September 21, 2023 13:52
@derekpierre
Copy link
Member Author

derekpierre commented Sep 21, 2023

An assumption I'm making:

Teacher nodes check two things about nodes when validating operator metadata:

  1. is operator bonded - https://github.com/derekpierre/nucypher/blob/taco-child/nucypher/network/nodes.py#L1042
  2. are they really staking - https://github.com/derekpierre/nucypher/blob/taco-child/nucypher/network/nodes.py#L1058

I assume the canonical spot for this information is indeed TACo root and NOT the child. Both are set on the root application, which subsequently updates the child application, so presumably, the root is the best place for that information. So I've left the above functions unchanged, i.e. they continue to use the TACoApplicationAgent (root application agent).

cc @vicky.z , @dnunez

@KPrasch
Copy link
Member

KPrasch commented Sep 21, 2023

I assume the canonical spot for this information is indeed TACo root and NOT the child. Both are set on the root application, which subsequently updates the child application, so presumably, the root is the best place for that information. So I've left the above functions unchanged, i.e. they continue to use the TACoApplicationAgent (root application agent).

Given that nodes are not permitted to start up without bonding state being synchronized with the child, this is a fair assumption. In this case, we want a node to become invalid as soon as it's unbonded, correct (no need to await state sync for unbonding)? We don't want to sample unqualified nodes after all, even before the unbonding state is bridged. Side question, does deauthorization of the TACo application on the staking-side result in a forced unbonding?

@derekpierre
Copy link
Member Author

derekpierre commented Sep 21, 2023

Given that nodes are not permitted to start up without bonding state being synchronized with the child, this is a fair assumption. In this case, we want a node to become invalid as soon as it's unbonded, correct (no need to await state sync for unbonding)? We don't want to sample unqualified nodes after all, even before the unbonding state is bridged.

👍 . Same thing for our OperatorBondedTracker, it uses root application agent.

Side question, does deauthorization of the TACo application on the staking-side result in a forced unbending?

Looks that way - https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/TACoApplication.sol#L445.

@vzotova / @cygnusv can confirm.

Copy link
Member

@vzotova vzotova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I'd say that additional 30-60 minutes before getting synchronized info to child - for me would be ok, that can simplify things a lot. But yeah most fast way to get new information - read from both sides of bridge

@KPrasch KPrasch merged commit 2ed9423 into nucypher:development Sep 21, 2023
7 checks passed
KPrasch pushed a commit that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

TACo protocol should use TACoChildApplication as a reference for staking information
4 participants