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

FIP-0076 fix: s/SectorDeals/SectorDealIDs in market state to disambiguate naming #930

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 29, 2024

SectorDeals is also used in BatchActivateDeals but has a different form there. This change simply renames the struct to disambiguate it. This also reflects how it's implemented in builtin-actors.


While I'm in here, can I ask for clarity on why this is a struct in the first place? It's introduced new complexity now that it's adopted "transparent" encoding in builtin-actors which has required new cbor-gen adjustments to make it work with go-state-types too. Why are we containing it in a struct in the first place and not just doing SectorDealIDs []DealID or omitting SectorDealIDs entirely and saying HAMT[Address]HAMT[SectorNumber][]DealID, which would both result in the same forms as they are currently implemented in both of the proposed builtin-actors and go-state-types but may afford a little bit of simplification in the code by removing an intermediary?

(Edit: here's what such simplification could look like: filecoin-project/builtin-actors#1510)

SectorDeals is also used in BatchActivateDeals but has a different form. This change simply renames the struct to disambiguate it. This also reflects how it's implemented in builtin-actors.
@jennijuju
Copy link
Member

The fip change lgtm to reflect the implementation.

(Cc @Stebalien @anorth for the question asked in PR description

@Stebalien
Copy link
Member

I agree with the change in filecoin-project/builtin-actors#1510, honestly. Wrapping in a struct makes some sense for method parameters and return values, but doesn't really add anything here (IMO).

@anorth
Copy link
Member

anorth commented Jan 29, 2024

Why are we containing it in a struct in the first place and not just doing SectorDealIDs []DealID or omitting SectorDealIDs entirely and saying HAMT[Address]HAMT[SectorNumber][]DealID, which would both result in the same forms as they are currently implemented in both of the proposed builtin-actors and go-state-types but may afford a little bit of simplification in the code by removing an intermediary?

Sure, they're all fine. I don't understand the problem. Pseudocode in the FIP isn't code code, implementations should represent it however makes sense to them. I would happily also switch to an IPLD-CBOR schema language if that was going to be clearly understandable to readers (it might! I'd love to!).

@anorth anorth merged commit 2538694 into filecoin-project:master Jan 29, 2024
1 check passed
rvagg added a commit to filecoin-project/builtin-actors that referenced this pull request Jan 30, 2024
rvagg added a commit to filecoin-project/builtin-actors that referenced this pull request Jan 30, 2024
rvagg added a commit to filecoin-project/builtin-actors that referenced this pull request Feb 2, 2024
github-merge-queue bot pushed a commit to filecoin-project/builtin-actors that referenced this pull request Feb 2, 2024
rjan90 pushed a commit to filecoin-project/builtin-actors that referenced this pull request Feb 8, 2024
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