Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

feat: implement fip-0029, support beneficiary #1571

Closed
wants to merge 37 commits into from

Conversation

hunjixin
Copy link

@hunjixin hunjixin commented Jan 28, 2022

This is an implementation for FIP-0029 PR #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.

@hunjixin hunjixin requested a review from a team as a code owner January 28, 2022 09:36
@hunjixin hunjixin marked this pull request as draft January 28, 2022 09:41
@hunjixin hunjixin marked this pull request as ready for review January 28, 2022 13:07
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.

Given the representation of the beneficiary address always being non-nil, and equal to the owner address by default, we must update the beneficiary address when the owner address is changed, if they are equal. Also it's hard to think about various cases when a proposed beneficiary change is active. I think we should delete any proposal when the owner changes. Please fix ChangeOwnerAddress to do this.

actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
@hunjixin hunjixin changed the title feat: implement fip-0028, support beneficiary feat: implement fip-0029, support beneficiary Feb 4, 2022
@steven004
Copy link
Contributor

@anorth , thanks for your comments, which are thoroughly considered and make things even simpler. Please review again.
And, please also see if a new branch (instead of master) needed to do this merge.

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 basically looks good to me. My comments and suggestions now are relatively minor.

I'd like to defer to @ZenGround0 for final approval though, and discussion about when and where to merge.

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_test.go Show resolved Hide resolved
actors/builtin/miner/miner_test.go Show resolved Hide resolved
@anorth anorth requested a review from ZenGround0 February 14, 2022 02:40
@ZenGround0
Copy link
Contributor

Landed in builtin actors, repo is deprecated

@ZenGround0 ZenGround0 closed this Jun 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants