-
Notifications
You must be signed in to change notification settings - Fork 58
feat(execution): create a package metadata object at publish time #9296
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
base: vm-lang/aa-auth/8116-feature-branch
Are you sure you want to change the base?
feat(execution): create a package metadata object at publish time #9296
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
|
Correct me if I'm wrong, are there any possible exploits present in the runtime ? |
iota-execution/latest/iota-verifier/src/authenticator_verifier.rs
Outdated
Show resolved
Hide resolved
crates/iota-framework/packages/iota-framework/sources/package_metadata.move
Show resolved
Hide resolved
Cargo.lock
Outdated
| version = "3.0.3" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" | ||
| source = "git+https://github.com/iotaledger/iota-sim.git?branch=tokio-1.46.1#20adc0b089bf9072268eea0f059fb8ea2461702f" |
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.
Do we expect this change, or should it be reverted?
I see several links updated to iota-sim.
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 pushed the changes after running a sim test and without re-running cargo check or similar. Fixed 9fc51cc
| /// | ||
| /// The `AuthenticatorInfo` will be attached to the account being built. | ||
| public fun builder( | ||
| authenticator: AuthenticatorInfoV1<AbstractAccount>, |
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.
We discussed an option of creating an account with a framework function:
public fun create_account_v1<T: key>(obj: T, info: AuthenticatorInfoV1<T>) {
create_account_impl(obj, info)
}So, maybe it would be better to leave the AuthenticatorInfoV1 generic parameter and not change the interface of the examples?
We don't really need to use PackageMetadataV1 here; it makes the account implementation and testing much more complex. AuthenticatorInfoV1 makes sense because we can easily limit the authenticator type that can be attached to an account.
Even if we leave the proof for a while, it will be cleaner to leave the generic parameter.
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.
To talk about
| ): AuthenticatorInfoV1<Account> { | ||
| public fun rotate_auth_info_v1(account_id: &mut UID, proof: AuthenticatorInfoV1CompatibilityProof) { |
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.
What is the reason for removing the return value?
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 return was introduced because drop was not used as ability for AuthenticatorInfoV1 . So, rotate_auth_info_v1 was never modified after adding drop to the abilities.
crates/iota-framework/packages/iota-framework/sources/package_metadata.move
Outdated
Show resolved
Hide resolved
crates/iota-framework/packages/iota-framework/sources/package_metadata.move
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| for (fn_name, account_type) in fns_metadata { | ||
| value.authenticator_metadata.push(AuthenticatorMetadataV1 { |
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.
Vec can't be used here as is; we need to add validation that it doesn't contain duplicates.
There is no guarantee that it doesn't; in this struct we don't know from where this information is taken.
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.
Moved the creation up, where it is sure that the verifier was run. Then I added a check for duplicates in the verifier 6878cb6
| /// wrapper using the collected module metadata, storage ID, runtime ID, | ||
| /// and package version and finally freezes it. If no relevant metadata | ||
| /// is found, the function exits without creating any package metadata. | ||
| fn create_and_freeze_package_metadata_if_present( |
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.
| fn create_and_freeze_package_metadata_if_present( | |
| fn create_and_freeze_package_metadata_v1_if_present( |
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? The inner logic can be easily modified to support a v2 in the future, I don't see necessary the creation of create_and_freeze_package_metadata_v2_if_present in that case.
| /// `Account` is a phantom type representing the account type which can be authenticated. | ||
| #[allow(unused_field)] | ||
| public struct AuthenticatorInfoV1<phantom Account: key> has copy, drop, store { | ||
| public struct AuthenticatorInfoV1 has copy, drop, store { |
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.
What did we decide about renaming this to more suitable name?
Like AuthenticatorDescriptor or similar?
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.
Should be done in a different PR!
crates/iota-framework/packages/iota-framework/sources/account.move
Outdated
Show resolved
Hide resolved
| /// The object id of the runtime package metadata object is derived from | ||
| /// this value. | ||
| storage_id: ID, | ||
| /// Runtime ID of the package represented by this metadata |
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 comment useful in this context?
Could we elaborate the meaning or remove it?
| use std::type_name::TypeName; | ||
|
|
||
| /// Key type for deriving the package metadata object address | ||
| public struct PackageMetadataKey has copy, drop, store {} |
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 it should be V1 as well?
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.
Nope, this can be used without versioning as we won't have the need to have 2 different keys for 2 different versions in the future.
Description of change
This PR removes the previous method for verifying the authenticate functions which worked at runtime. Then, it introduces the creation of a Package Metadata immutable object during a package publishing/upgrade. Finally, it allows to use such metadata to create
AuthenticatorInfoV1instances for accounts.Links to any relevant issues
Fixes #8861
Fixes #8862
How the change has been tested