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

miner actor implemented #486

Merged
merged 26 commits into from
Jun 11, 2020
Merged

miner actor implemented #486

merged 26 commits into from
Jun 11, 2020

Conversation

dutterbutter
Copy link
Contributor

@dutterbutter dutterbutter commented Jun 8, 2020

Summary of changes

Changes introduced in this pull request:

  • Implements miner actor

Reference issue to close (if applicable)

Closes #265

Other information and links

@austinabell
Copy link
Contributor

I lied about doing all in one commit, figured it might be easier to reverse review if not all at once and I don't want to lose changes and redo any of this: 80e1bd7

I will continue along and do the other ~1/2 tomorrow and can merge the two commits if it makes it easier

@austinabell
Copy link
Contributor

p2 6d97b3d

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

approval but keep in mind it's based on my own changes at the end there


impl DeadlineInfo {
pub fn new(period_start: ChainEpoch, deadline_idx: usize, current_epoch: ChainEpoch) -> Self {
if deadline_idx < WPOST_PERIOD_DEADLINES {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it is worth checking the above restrictions like period_start > current_epoch here and throwing an Err?

Copy link
Contributor

Choose a reason for hiding this comment

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

can't do that, would be inconsistent (unless I'm missing something)

vm/actor/src/builtin/miner/deadlines.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Show resolved Hide resolved
vm/actor/src/builtin/miner/policy.rs Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner/mod.rs Show resolved Hide resolved
@dutterbutter dutterbutter requested a review from ec2 June 11, 2020 16:00
@dutterbutter dutterbutter merged commit f38a001 into master Jun 11, 2020
@dutterbutter dutterbutter deleted the dustin/miner branch June 11, 2020 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement miner actor and update actors accordingly
4 participants