feat: implement UpgradeManager and UpgradeNote - #3299
Conversation
zeapoz
left a comment
There was a problem hiding this comment.
Looks good to me in general!
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I mostly reviewed the non-test code and left a couple of small comments inline. The main one is about the location of the component. I don't love it being in access or extensions. capabilities is better - though, in a way, every account component adds a "capability" to the account. I actually don't mind utils for this (and for NoteCreator) but I'm fine with capabilities too.
Another thing I realized is that we either need to add the UPGRADE note to allowlists of all network accounts. Or we need to enable the ability to update this list of notes. The latter is covered by #3240 and I've assigned @Fumuran to it.
| pub struct UpgradeNoteStorage { | ||
| code_upgrade_commitment: Word, | ||
| storage_upgrade_commitment: Word, | ||
| } |
There was a problem hiding this comment.
This is fine for now, but when we actually get to the point when we want to make it work, we'll somehow need to include the underlying code and storage data that matches these commitments. Maybe this could be done with attachments (i.e., one attachment for code update and another one for storage update).
There was a problem hiding this comment.
Right, I missed that we can't easily provide advice for network notes, so this doesn't work the way it is currently setup. If we have #3240, it may be best to not implement the UpgradeNote now and instead leave that for later. I think it would basically need to carry the new account code and account storage in its note storage and then compute the commitments and insert into advice map upon consumption. But given #3240, we can fortunately deal with this later.
mmagician
left a comment
There was a problem hiding this comment.
In terms of module naming, I'd probably place this in its own standards/upgrade module, rather than trying to make it also fit the NoteCreator
Otherwise LGTM, though the PR description mentions an UPGRADE note standard which is missing here (a version of it is inlined in tests, do we want to make this a proper note standard now or defer to later?)
|
Per @mmagician's suggestion, I changed it to |
Implements
UpgradeManageraccount component gated byAuthorityand a correspondingUpgradeNotethat triggers the upgrade. For now this is purely the infrastructure without any real functionality.Went with the
extensionsmodule, but alternatives could becapabilities. I tried to come up with a name that could also be the home of the genericNoteCreator. I would like to avoidutilssince it is so generic, though the options I named are also pretty generic.Does not yet add the
UpgradeManagerto any of the convenience account builders.Do we need the ability to upgrade code or storage on the agglayer bridge? If so, the bridge does not have
Authorityand so it wouldn't be able to useUpgradeManagerorUpgradeNoteand would need a custom version to use something likeassert_sender_is_bridge_adminfor upgrade authorization. cc @mmagicianpart of #2183