-
Notifications
You must be signed in to change notification settings - Fork 24
2580 migrate schemas to schemas 20 #2603
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
2580 migrate schemas to schemas 20 #2603
Conversation
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.
Removed packages that were only being used for schemas pallet RPCs that are now removed
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.
None of this was being used except for one custom RPC in the schemas pallet that allows a user to validate an Avro schema. However, since the chain itself does not do any validation of schema models (aside from size, and some extremely basic Frequency-specific Parquet validation), this seems very unnecessary. The onus is therefore on both the owner of a schema and Governance to validate its correctness before uploading to the chain.
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 actually see no need for the messages pallet to have a custom RPC identical to the one we just removed from the schemas pallet. Upcoming PR may remove this one as well...
|
|
||
| /// Retrieve a schema by id | ||
| fn get_schema_by_id(schema_id: SchemaId) -> Option<SchemaResponse>; | ||
| fn get_schema_by_id(schema_id: SchemaId) -> Option<SchemaResponseV2>; |
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.
same as above; this seems unnecessarily duplicative; the schemas runtime API already has this method
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 doesn't looks like backwards compatible. I think maybe we want to keep this and add a new function and then remove the old one later
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.
All custom RPCs for schemas pallet have been removed, so there is no RPC client or server for this pallet.
| StorageMap<_, Twox64Concat, IntentGroupId, IntentGroup<T>, OptionQuery>; | ||
|
|
||
| #[pallet::genesis_config] | ||
| pub struct GenesisConfig<T: Config> { |
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.
ALL state storage is now settable by the genesis config; this makes it much easier to set up tests.
| /// - Reads/Writes from checking/settings the on-chain storage version are accounted for | ||
| pub type MigrateV4ToV5<T> = frame_support::migrations::VersionedMigration< | ||
| 4, // The migration will only execute when the on-chain storage version is 4 | ||
| 5, // The on-chain storage version will be set to 5 after the migration is complete |
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 is cool
saraswatpuneet
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.
gave a read through, tested try runtime locally so that checks out. Some nits and it would be nice to have a one migration test
shannonwells
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.
Looks good to me - only non-blocking comments/questions/suggestions
- I read through the changes
- I verified that create_schema_v4 is filtered out of mainnet
- I verified that creating_schema_via_governance checks for correct origin
- I checked that the error conditions for the new extrinsics were already unit tested.
- I did not look at: generated weights or migration code.
| // }, | ||
| // }; | ||
|
|
||
| /// Schema genesis format |
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's the plan for this commented-out 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.
This is informational--since Wil originally developed this as a pure JS tool, I didn't feel like introducing the overhead to convert to Typescript, but I wanted to document the structure of the genesis config as defined by the Rust implementation.
That said, I'd be happy to convert to Typescript if we think that would be cleaner. It's just that it adds a bunch to the tooling required. But, since this is not something that runs in CI, that might be okay.
|
|
||
| /// a method to return all versions of a schema name with their schemaIds | ||
| /// Warning: Must only get called from RPC, since the number of DB accesses is not deterministic | ||
| pub fn get_schema_versions(schema_name: Vec<u8>) -> Option<Vec<SchemaVersionResponse>> { |
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.
suggestion: could make this only a crate-visible function to help communicate this intended restriction. It doesn't prevent its being called by other crate members, but it can't be pulled in outside of the crate that way.
| pub fn get_schema_versions(schema_name: Vec<u8>) -> Option<Vec<SchemaVersionResponse>> { | |
| pub(crate) fn get_schema_versions(schema_name: Vec<u8>) -> Option<Vec<SchemaVersionResponse>> { |
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 think that would that prevent it from being called by an extrinsic within the crate, though, would it? That's the main point here.
Co-authored-by: Shannon Wells <shannonwells@users.noreply.github.com>
aramikm
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.
Added some comments we should also create an issue about updating js/schemas library
|
|
||
| /// Retrieve a schema by id | ||
| fn get_schema_by_id(schema_id: SchemaId) -> Option<SchemaResponse>; | ||
| fn get_schema_by_id(schema_id: SchemaId) -> Option<SchemaResponseV2>; |
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 doesn't looks like backwards compatible. I think maybe we want to keep this and add a new function and then remove the old one later
Co-authored-by: Aramik <aramikm@gmail.com>
enddynayn
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.
🚀 impressive work!
fb63aab
into
feat/schemas-permissions-development
Goal
The goal of this PR is to update the types, extrinsics, and runtime APIs for the Schemas pallet to conform to the new design
Closes #2580
Checklist