Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-0088: Add support for upgradable actors #873
FIP-0088: Add support for upgradable actors #873
Changes from all commits
16964ee
30339ac
1f78cb5
ae8ce13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why must deployed actors implement it? It's not invoked on deployed actors, it's invoked on the new code. The algorithm below does not specify any check that the deployed actors implement this entry point
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.
Nit: I don't think these Rust codegen things are relevant to the FIP.
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.
This first sentence is a bit unclear
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.
Nit: the entry point and syscall are presented in the opposite order to which they happen in during an upgrade, which makes grokking the flow a bit harder. I drew a completely wrong idea about the upgrade method until I finished reading the syscall spec.
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.
The algorithm below specifies the opposite order of things: the code is changed then
upgrade
is invoked (in the new code). The opposite order makes sense, since the old code can't possibly know what state schema is required by the new code, while the new code can know both schemas.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.
Please be very clear which code is invoked here. Since the prior step changed the code CID, I'm assuming it's the new code, but "target actor" also makes it sound like the old code.
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.
I don't understand from the text above how this is enforced.
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.
Update: the text above doesn't say this, but I now do understand. It's opt-in because an actor code must have some path that invokes the
upgrade
syscall in order to be upgradeable. The stuff about requiring theupgrade
entrypoint still seems wrong, but the fact that actors can only upgrade themselves means that an actor can be determined to be non-upgradeable by lack of such a call (up to malicious obfuscation, etc).I suggest this mechanism and property be spelled out more clearly.
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.
Is this true? I'm curious how this change may impact the logic around timing and resource use for network upgrades, as well as the prioritization of certain changes.
If actors' code can be much more efficiently upgraded, shouldn't we technically be able to push more updates/small changes more quickly?
This change may not effect cryptoeconomics, but I might request we add a line about affecting the logic of what gets included for upgrades. Ultimately a small nit; the approval of the draft shouldn't be gated on this topic.
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.
@kaitlin-beegle I don't think I get why those are incentive considerations rather than product considerations.