Skip to content

EconomicsFactory/Economics Removal#3241

Merged
KPrasch merged 3 commits intonucypher:developmentfrom
derekpierre:here-be-zombies
Sep 22, 2023
Merged

EconomicsFactory/Economics Removal#3241
KPrasch merged 3 commits intonucypher:developmentfrom
derekpierre:here-be-zombies

Conversation

@derekpierre
Copy link
Copy Markdown
Member

@derekpierre derekpierre commented Sep 21, 2023

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Removal of obsolete EconomicsFactory/Economics classes.

Issues fixed/closed:

  • Fixes #...

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?

derekpierre added a commit to derekpierre/nucypher that referenced this pull request Sep 21, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 21, 2023

Codecov Report

Merging #3241 (cbfb8a7) into development (21aefb7) will increase coverage by 0.03%.
Report is 1 commits behind head on development.
The diff coverage is 56.25%.

@@               Coverage Diff               @@
##           development    #3241      +/-   ##
===============================================
+ Coverage        80.38%   80.42%   +0.03%     
===============================================
  Files              113      112       -1     
  Lines            11300    11260      -40     
===============================================
- Hits              9084     9056      -28     
+ Misses            2216     2204      -12     
Flag Coverage Δ
acceptance 91.03% <50.00%> (-0.07%) ⬇️
integration 73.26% <100.00%> (+0.02%) ⬆️
unit 57.57% <100.00%> (-0.04%) ⬇️

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

Files Changed Coverage Δ
tests/acceptance/conftest.py 100.00% <ø> (ø)
tests/acceptance/agents/test_token_agent.py 27.69% <46.15%> (+1.12%) ⬆️
nucypher/blockchain/eth/actors.py 83.74% <100.00%> (-0.10%) ⬇️
nucypher/cli/types.py 71.13% <100.00%> (-0.30%) ⬇️
tests/acceptance/agents/test_contract_agency.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@derekpierre derekpierre changed the title [WIP] EconomicsFactory/Economics Removal EconomicsFactory/Economics Removal Sep 21, 2023
@derekpierre derekpierre marked this pull request as ready for review September 21, 2023 16:32
@derekpierre derekpierre self-assigned this Sep 21, 2023

__min_authorization = TToken.from_units(Economics._default_min_authorization).to_tokens()

__min_authorization = TToken(40_000, "T").to_tokens() # TODO right spot for this?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the use for this anyway? The min authorization is whatever is in the contract, and we don't use the nucypher CLI for deployment or staking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's used in a CLI option as a default value - https://github.com/nucypher/nucypher/blob/development/nucypher/cli/options.py#L65 - which is used as an Ursula parameter for specify the seed node minimum stake.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

huh, but I'm wondering why is that a CLI parameter. The only thing that should matter is what's in the contract, so I don't see how a different value can be used

Copy link
Copy Markdown
Member Author

@derekpierre derekpierre Sep 22, 2023

Choose a reason for hiding this comment

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

The CLI parameter allows node to specify a higher stake requirement for its seed node teacher than the minimum (trust mitigation I guess... 🤷), but you are right that perhaps the logic can be reworked so that if a value is not specified, the min value is obtained from the contract.

def policy_value(application_economics, policy_rate):
value = policy_rate * application_economics.min_operator_seconds
def policy_value(policy_rate):
value = policy_rate * MIN_OPERATOR_SECONDS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this constant represent?

Copy link
Copy Markdown
Member Author

@derekpierre derekpierre Sep 22, 2023

Choose a reason for hiding this comment

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

It's the minimum amount of time before an operator can be changed. It's representative of the used in the TACoApplication contract.

Copy link
Copy Markdown
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

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

You know how much I love red lines 🤠

__ACCOUNT_CACHE = list()

# Defaults
DEFAULT_ECONOMICS = Economics()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🪓

@KPrasch KPrasch merged commit 4e0ead5 into nucypher:development Sep 22, 2023
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.

5 participants