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

fix: state: rename Actor.Address and only use it for f4 addresses #12155

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jun 27, 2024

Related Issues

Per the FIP, the top-level actor address field should only be used for delegated addresses. Unfortunately, the FIP's design was changed but neither lotus genesis code nor the field name were updated to reflect this. Fortunately, all the migration code (on mainnet, at least), has correctly left this field unset/unchanged (except for actors with f4 addresses).

Proposed Changes

  1. Rename Actor5.Address to Actor5.DelegatedAddress.
  2. Only set it when it's an f4 address.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@Stebalien Stebalien force-pushed the fix/delegated-address-genesis branch from 4da459f to 34f1318 Compare June 27, 2024 00:13
@Stebalien Stebalien requested a review from ZenGround0 June 27, 2024 00:13
@jennijuju
Copy link
Member

AH. - I see what you meant now! Thanks

Stebalien added a commit to filecoin-project/go-state-types that referenced this pull request Jun 27, 2024
Per the FIP [1], the top-level actor address field should only be used
for delegated addresses. Unfortunately, the FIP's design was changed [2]
but neither lotus genesis code nor the field name were updated to
reflect this. Fortunately, all the migration code (on mainnet, at
least), has correctly left this field unset/unchanged (except for actors
with f4 addresses).

[1]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#new-lookup_delegated_address-syscall-and-state-changes
[2]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#recording-other-addresses-in-the-actorstate-root
@Stebalien Stebalien force-pushed the fix/delegated-address-genesis branch from 34f1318 to f72ae9c Compare June 27, 2024 00:17
@Stebalien
Copy link
Member Author

In retrospect, we probably didn't make the rename change in the past as it is technically an API breaking change...

@Stebalien
Copy link
Member Author

Equivalent change in the state types: filecoin-project/go-state-types#279 (review)

@Stebalien
Copy link
Member Author

The flaky test: #12001

@Stebalien Stebalien merged commit 6d403bf into master Jun 27, 2024
77 of 78 checks passed
@Stebalien Stebalien deleted the fix/delegated-address-genesis branch June 27, 2024 00:48
ZenGround0 pushed a commit to filecoin-project/go-state-types that referenced this pull request Jun 27, 2024
aarshkshah1992 pushed a commit that referenced this pull request Jun 27, 2024
…2155)

Per the FIP [1], the top-level actor address field should only be used
for delegated addresses. Unfortunately, the FIP's design was changed [2]
but neither lotus genesis code nor the field name were updated to
reflect this. Fortunately, all the migration code (on mainnet, at
least), has correctly left this field unset/unchanged (except for actors
with f4 addresses).

[1]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#new-lookup_delegated_address-syscall-and-state-changes
[2]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#recording-other-addresses-in-the-actorstate-root
aarshkshah1992 pushed a commit that referenced this pull request Jun 27, 2024
…2155)

Per the FIP [1], the top-level actor address field should only be used
for delegated addresses. Unfortunately, the FIP's design was changed [2]
but neither lotus genesis code nor the field name were updated to
reflect this. Fortunately, all the migration code (on mainnet, at
least), has correctly left this field unset/unchanged (except for actors
with f4 addresses).

[1]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#new-lookup_delegated_address-syscall-and-state-changes
[2]: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0048.md#recording-other-addresses-in-the-actorstate-root
@rjan90 rjan90 mentioned this pull request Aug 31, 2024
8 tasks
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.

4 participants