Skip to content

Auto-detect HW accounts in TransactingPower. Finalize basic staking via contract CLI functionality.#1402

Merged
cygnusv merged 25 commits intonucypher:biznagafrom
cygnusv:allocation-registry
Oct 22, 2019
Merged

Auto-detect HW accounts in TransactingPower. Finalize basic staking via contract CLI functionality.#1402
cygnusv merged 25 commits intonucypher:biznagafrom
cygnusv:allocation-registry

Conversation

@cygnusv
Copy link
Member

@cygnusv cygnusv commented Oct 8, 2019

@cygnusv cygnusv changed the base branch from master to biznaga October 8, 2019 00:30
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #1402 into biznaga will decrease coverage by 0.01%.
The diff coverage is 78.43%.

Impacted file tree graph

@@             Coverage Diff             @@
##           biznaga    #1402      +/-   ##
===========================================
- Coverage    83.17%   83.15%   -0.02%     
===========================================
  Files           72       72              
  Lines        10709    10836     +127     
===========================================
+ Hits          8907     9011     +104     
- Misses        1802     1825      +23
Impacted Files Coverage Δ
nucypher/blockchain/eth/utils.py 67.39% <100%> (+4.89%) ⬆️
nucypher/blockchain/eth/token.py 89.92% <100%> (+0.43%) ⬆️
nucypher/blockchain/eth/agents.py 90.1% <100%> (+0.1%) ⬆️
nucypher/blockchain/eth/deployers.py 88.51% <100%> (+0.09%) ⬆️
nucypher/utilities/sandbox/constants.py 98.43% <100%> (+0.02%) ⬆️
nucypher/blockchain/eth/clients.py 63.66% <50%> (-0.08%) ⬇️
nucypher/cli/deploy.py 77.71% <50%> (-0.14%) ⬇️
nucypher/crypto/powers.py 92% <60%> (-2.58%) ⬇️
nucypher/cli/painting.py 74.5% <61.53%> (-4.04%) ⬇️
nucypher/cli/actions.py 74.28% <61.53%> (+0.79%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0976b2b...d2de49c. Read the comment docs.

@cygnusv cygnusv changed the title [WIP] Redesign Allocation registry Auto-detect HW accounts in TransactingPower. Finalize basic staking via contract CLI functionality. Oct 9, 2019
@KPrasch KPrasch self-requested a review October 10, 2019 03:01
@KPrasch
Copy link
Member

KPrasch commented Oct 10, 2019

looks like it's passing :-) why else do you you anticipate including?

@cygnusv cygnusv marked this pull request as ready for review October 14, 2019 16:41
cygnusv added a commit to cygnusv/nucypher that referenced this pull request Oct 15, 2019
@cygnusv cygnusv force-pushed the allocation-registry branch from 9b0bd65 to 89fe1be Compare October 15, 2019 20:12
@cygnusv cygnusv force-pushed the allocation-registry branch from ad641eb to 146fb9d Compare October 16, 2019 16:43
@click.option('--lock-until', help="Period to release re-staking lock", type=click.IntRange(min=0))
@click.option('--beneficiary-address', help="Address of a pre-allocation beneficiary", type=EIP55_CHECKSUM_ADDRESS)
@click.option('--allocation-filepath', help="Path to individual allocation file", type=EXISTING_READABLE_FILE)
@click.option('--all', help="List all stakes, including inactive", is_flag=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

--all sounds like a too general word for this option. Maybe --show-inactive, or --with-inactive?

Copy link
Member Author

@cygnusv cygnusv Oct 17, 2019

Choose a reason for hiding this comment

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

I agree, although @szotov told me something today about how these stakes are not really "inactive", so perhaps we have to rethink this option altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this needs a rename. Does this have an issue open?

``--allocation-filepath /path/to/allocation_registry.json``.
As an alternative to the ``--allocation-filepath`` flag, preallocation users
can directly specify their beneficiary and staking contract addresses with the
``--beneficiary-address ADDRESS`` and ``--staking-address ADDRESS``, respectively.
Copy link
Contributor

Choose a reason for hiding this comment

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

So, staking address is address of the contract which will stake for this beneficiary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly.

* Remove --escrow flag
* No account selection; use individual allocation file or explicit addresses
@cygnusv cygnusv force-pushed the allocation-registry branch from 21fa779 to c157a51 Compare October 19, 2019 22:31
return r

def __eq__(self, other) -> bool:
# TODO: Is this right? Two stakes from different accounts, durations, etc, but of the same value, are equal?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seems off...

self.start_datetime = datetime_at_period(period=first_locked_period,
seconds_per_period=self.economics.seconds_per_period)
seconds_per_period=self.economics.seconds_per_period,
start_of_period=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing for a boolean flag. Would it be better as is_start_of_period?

@click.option('--lock-until', help="Period to release re-staking lock", type=click.IntRange(min=0))
@click.option('--beneficiary-address', help="Address of a pre-allocation beneficiary", type=EIP55_CHECKSUM_ADDRESS)
@click.option('--allocation-filepath', help="Path to individual allocation file", type=EXISTING_READABLE_FILE)
@click.option('--all', help="List all stakes, including inactive", is_flag=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this needs a rename. Does this have an issue open?

Copy link
Contributor

@michwill michwill left a comment

Choose a reason for hiding this comment

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

Approving, but we need to address the situation with timedeltas elsewhere (I understand, you didn't want to include that in the scope of this PR)

@cygnusv cygnusv merged commit a4b32c0 into nucypher:biznaga Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants