Skip to content

chore!: unify miner, market and verifreg builtin wrappers to v16#12960

Closed
rvagg wants to merge 10 commits intomasterfrom
rvagg/unify-builtintypes16
Closed

chore!: unify miner, market and verifreg builtin wrappers to v16#12960
rvagg wants to merge 10 commits intomasterfrom
rvagg/unify-builtintypes16

Conversation

@rvagg
Copy link
Copy Markdown
Member

@rvagg rvagg commented Mar 17, 2025

#12932 (comment)

This is the cost of unifying the miner types, it has flow-on effects for market and verifreg. It cleans some things up and there's probably a lot of opportunities in here to do more disentanglement (I haven't, this is just a quick rework of all the broken bits).

Is it worth it?

Comment on lines +42 to +44
market9 "github.com/filecoin-project/go-state-types/builtin/v16/market"
miner9 "github.com/filecoin-project/go-state-types/builtin/v16/miner"
verifreg9 "github.com/filecoin-project/go-state-types/builtin/v16/verifreg"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't rename these because they need to be checked first, this is for nv17 invariant checks and I'm not sure it's OK to do this

@BigLep BigLep moved this from 📌 Triage to ⌨️ In Progress in FilOz Mar 18, 2025
@rvagg rvagg force-pushed the rvagg/fip-0100 branch 5 times, most recently from 3bf4c3f to e0c7149 Compare March 18, 2025 03:09
@Stebalien
Copy link
Copy Markdown
Member

Yeah, let's punt this for now. But I think the right way to do this is to alias the state types in the abstraction (chain/actors/builtin/...) and avoid importing go-state-types/builtin/vN outside of said abstractions.

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Mar 18, 2025

Agreed, but last time I proposed doing something like that it involved pulling in a lot more types than we reference and I got push-back. But I'm happy to go further than this.

@rvagg rvagg force-pushed the rvagg/fip-0100 branch 7 times, most recently from eadab96 to 286bb13 Compare March 19, 2025 01:48
@rvagg rvagg force-pushed the rvagg/fip-0100 branch 3 times, most recently from 72f5d6f to e432ffb Compare March 19, 2025 02:18
Base automatically changed from rvagg/fip-0100 to master March 19, 2025 02:43
@magik6k
Copy link
Copy Markdown
Contributor

magik6k commented Apr 17, 2025

Is it worth it?

Just flagging that this will also cause pain in boost, curio, cidgravity-gateway, and probably many other projects which use the current aliases. Really that situation is quite unfortunate, and I'd also like to one day fix this, but I wouldn't do this without very strong motivation.

@rvagg rvagg closed this Apr 19, 2025
@github-project-automation github-project-automation Bot moved this from ⌨️ In Progress to 🎉 Done in FilOz Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

5 participants