-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: migrations: function for migrating builtin actors with only code changes #8815
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
Conversation
b4577fb to
24a2f4c
Compare
24a2f4c to
b28cb2a
Compare
arajasek
left a comment
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.
Testy test, pls.
6e1a894 to
fdb3547
Compare
87c7fd4 to
526acd7
Compare
526acd7 to
3c4792d
Compare
arajasek
left a comment
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.
Generally looks good, but some things need to change.
Can you also please update actors_version_checklist to reflect this new flow under the Migration instructions.
| type ChainIO interface { | ||
| ChainReadObj(context.Context, cid.Cid) ([]byte, error) | ||
| ChainHasObj(context.Context, cid.Cid) (bool, error) | ||
| ChainPutObj(context.Context, blocks.Block) error |
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.
@magik6k We had to do this just so we could use the itest framework for a migration test, which is not really what it's intended for, but is better than any of the existing frameworks we have (I think). Questions for you:
- Do you absolutely hate it and refuse to merge this PR?
- Is there a way to not expose this method over the JSON-RPC, and have it just for things like this test?
- Do you have a better way to write this test?
09cb375 to
801c670
Compare
arajasek
left a comment
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.
Still needs:
10638a3 to
e84a778
Compare
e84a778 to
3f712c3
Compare
arajasek
left a comment
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.
Landing lest it rot, but we do need Magik answer re: #8815 (comment).
Related Issues
Closes #8780
Proposed Changes
Adds
LiteMigration()toupgrades.gowhich should allow for easy upgrades if actors code needs to change but state does not. Example provided above the function to perform all the migration duties. Checkactors_version_checklist.mdfor the rest of the steps.Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps