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

feat:implement beneficiary fip0029 #496

Merged
merged 6 commits into from
Aug 29, 2022

Conversation

hunjixin
Copy link
Contributor

@hunjixin hunjixin commented Jul 14, 2022

This is an implementation for FIP-0029 PR filecoin-project/specs-actors#1571 to provide Beneficiary address support. In this implementation, version and upgrading is not considered, that is, directly put the code change into the current version. The back-compliance could be done once the next version content is determined.

Closes #422

@anorth anorth requested review from anorth, arajasek and Kubuxu July 14, 2022 23:03
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great so far. Could you please port the tests too?

actors/miner/src/beneficiary.rs Outdated Show resolved Hide resolved
actors/miner/src/beneficiary.rs Outdated Show resolved Hide resolved
actors/miner/src/state.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
@hunjixin
Copy link
Contributor Author

@anorth could give a test example, i dont know how to write test in this project

@anorth
Copy link
Member

anorth commented Jul 19, 2022

actors/miner/src/lib.rs Outdated Show resolved Hide resolved
@ZenGround0 ZenGround0 self-requested a review July 26, 2022 06:27
@hunjixin
Copy link
Contributor Author

why this pr's ci not running?

@anorth
Copy link
Member

anorth commented Jul 27, 2022

why this pr's ci not running?

It's possibly related to it being from a fork. @ZenGround0 @aarshkshah1992 do we have experience with this from other contributions?

@ZenGround0
Copy link
Contributor

ZenGround0 commented Aug 1, 2022

Thanks to @anorth and @galargh #510 is now merged and CI should run on next push. Give it a try and let me know if there's still a problem.

If push doesn't work try taking it out of draft and pushing too, and update us here about what (doesn't) work.

@hunjixin
Copy link
Contributor Author

hunjixin commented Aug 2, 2022

Thanks to @anorth and @galargh #510 is now merged and CI should run on next push. Give it a try and let me know if there's still a problem.

If push doesn't work try taking it out of draft and pushing too, and update us here about what (doesn't) work.

works fine

@hunjixin hunjixin force-pushed the master branch 2 times, most recently from af42b15 to 2f56193 Compare August 2, 2022 07:37
@anorth
Copy link
Member

anorth commented Aug 2, 2022

Thanks, I will review again now the tests are there

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks for the tests, great work.

Some minor comments, and a request for one new test to demonstrate what happens when the owner and beneficiary attempt to reduce the quota.

actors/miner/src/beneficiary.rs Show resolved Hide resolved
actors/miner/src/beneficiary.rs Show resolved Hide resolved
actors/miner/tests/util.rs Outdated Show resolved Hide resolved
actors/miner/tests/util.rs Show resolved Hide resolved
actors/miner/tests/withdraw_balance.rs Outdated Show resolved Hide resolved
actors/miner/tests/change_beneficiary_test.rs Show resolved Hide resolved
actors/miner/tests/change_beneficiary_test.rs Outdated Show resolved Hide resolved
actors/miner/tests/change_beneficiary_test.rs Outdated Show resolved Hide resolved
actors/miner/tests/change_beneficiary_test.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
@hunjixin hunjixin force-pushed the master branch 2 times, most recently from 3687586 to e501e28 Compare August 18, 2022 02:16
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Show resolved Hide resolved
@hunjixin hunjixin force-pushed the master branch 3 times, most recently from fce9db4 to db0e8b7 Compare August 26, 2022 09:43
@ZenGround0
Copy link
Contributor

Unfortunately I haven't given a thorough review yet and I'll be offline for a week but the high level looks good and the testing looks great. If necessary I will do a review after the fact so don't block on me if you need to merge before then.

@hunjixin hunjixin force-pushed the master branch 3 times, most recently from 16c2f21 to 8cd5400 Compare August 29, 2022 01:19
@anorth anorth merged commit 95f952a into filecoin-project:master Aug 29, 2022
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
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.

Implement FIP-0029: Beneficiary address
5 participants