-
Notifications
You must be signed in to change notification settings - Fork 37
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: make ClaimExtensionRequest provider type match builtin-actors #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested:
before
ID Provider Client Data Size TermMin TermMax TermStart Sector
1 1000 102 baga6ea4seaqavywo6dfcwtit4trep6zwmc3cu5zwvprsmvvnwj6fypqjlf7esmy 8388608 3675537 3934737 0 0
6 1000 1006 baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq 2048 518400 5256000 607 3
8 1000 1007 baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq 2048 518400 1555200 2936 4
After:
ID Provider Client Data Size TermMin TermMax TermStart Sector
1 1000 102 baga6ea4seaqavywo6dfcwtit4trep6zwmc3cu5zwvprsmvvnwj6fypqjlf7esmy 8388608 3675537 3934737 0 0
6 1000 1006 baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq 2048 518400 5256000 607 3
8 1000 1007 baga6ea4seaqpfonfro6j3gcw4uxkxbivlqn2fghx5dpulc6sbi5noz7bcvzmuiq 2048 518400 5256000 2936 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please make the corresponding fix to all appropriate actors versions (appears to be v10, v11, and v12).
Can be a separate PR if we want to land this stat.
Thanks @rvagg. Please follow up with a PR directly to the FIP in question to correct it to match the implementation. This doesn't need a governance process beyond normal PR review. |
2fb60e9
to
773cc89
Compare
FIP says Address, builtin-actors says ActorID for both ClaimExtensionRequest and AllocationRequest. This mismatch has been in place since v10. Ref: filecoin-project/builtin-actors@686a2f2
773cc89
to
94add2d
Compare
Thanks for digging deeper @arajasek, you're right that this is only since v10 and not v9 like I suggested. The commit didn't land until after v10: filecoin-project/builtin-actors@686a2f2 I've updated this to apply the same treatment to v10+ types (including comments). |
FIP says Address, builtin-actors says ActorID for both ClaimExtensionRequest and AllocationRequest.
FIP: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0045.md#creation-of-allocations-and-extension-of-claims-from-datacap-transfer
This mismatch has been in place since v9, but I'm not sure which way this should fall except that
AllocationRequest
has been working as anActorID
all this time so we probably don't want to break that.#76 was filled as per the request was to match builtin-actors in #57. But this PR landed before a change to builtin-actors that switched both of these from
Address
toActorID
: filecoin-project/builtin-actors#936. To #76 had it both ways, builtin-actors probably matched the FIP at the time withAddress
, but has been operating asActorID
ever since.My assumption is that we should align these to builtin-actors and do a retroactive FIP of some kind to clarify types of both of these.