search

filecoin-project / go-state-types

Primitive and low level types used in chain state and actor method parameters
Other
25 stars 35 forks source link

fix: make ClaimExtensionRequest provider type match builtin-actors #254

Closed rvagg closed 4 months ago

rvagg commented 4 months ago

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 an ActorID all this time so we probably don't want to break that.

https://github.com/filecoin-project/go-state-types/pull/76 was filled as per the request was to match builtin-actors in https://github.com/filecoin-project/go-state-types/issues/57. But this PR landed before a change to builtin-actors that switched both of these from Address to ActorID: https://github.com/filecoin-project/builtin-actors/pull/936. To #76 had it both ways, builtin-actors probably matched the FIP at the time with Address, but has been operating as ActorID 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.

anorth commented 4 months ago

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.

rvagg commented 4 months ago

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: https://github.com/filecoin-project/builtin-actors/commit/686a2f2497941783a34265224554dbeed69bd1b0

I've updated this to apply the same treatment to v10+ types (including comments).