Skip to content

Conversation

@JoeCap08055
Copy link
Collaborator

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

  • Updated Pallet Readme?
  • Updated js/api-augment for Custom RPC APIs?
  • Design doc(s) updated?
  • Unit Tests added?
  • e2e Tests added?
  • Benchmarks added?
  • Spec version incremented?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

@JoeCap08055 JoeCap08055 marked this pull request as ready for review September 15, 2025 21:06
Copy link
Collaborator Author

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>;
Copy link
Collaborator Author

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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> {
Copy link
Collaborator Author

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.

@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 42.81525% with 195 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pallets/schemas/src/migration/v5.rs 0.00% 102 Missing ⚠️
pallets/schemas/src/lib.rs 69.72% 56 Missing ⚠️
common/primitives/src/schema.rs 29.41% 24 Missing ⚠️
pallets/schemas/src/types.rs 28.57% 10 Missing ⚠️
pallets/schemas/src/migration/v4/mod.rs 0.00% 3 Missing ⚠️
Files with missing lines Coverage Δ
pallets/messages/src/rpc/src/lib.rs 96.07% <100.00%> (ø)
pallets/stateful-storage/src/lib.rs 78.60% <100.00%> (ø)
pallets/schemas/src/migration/v4/mod.rs 0.00% <0.00%> (ø)
pallets/schemas/src/types.rs 84.04% <28.57%> (-1.53%) ⬇️
common/primitives/src/schema.rs 37.86% <29.41%> (-5.89%) ⬇️
pallets/schemas/src/lib.rs 80.10% <69.72%> (-10.37%) ⬇️
pallets/schemas/src/migration/v5.rs 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 15, 2025
/// - 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is cool

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a 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

Copy link
Collaborator

@shannonwells shannonwells left a 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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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>> {
Copy link
Collaborator

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.

Suggested change
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>> {

Copy link
Collaborator Author

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>
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 17, 2025
Copy link
Collaborator

@aramikm aramikm left a 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>;
Copy link
Collaborator

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>
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 18, 2025
@github-actions github-actions bot removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 18, 2025
Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 impressive work!

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 18, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Sep 18, 2025
@JoeCap08055 JoeCap08055 merged commit fb63aab into feat/schemas-permissions-development Sep 18, 2025
29 of 32 checks passed
@JoeCap08055 JoeCap08055 deleted the 2580-migrate-schemas-to-schemas-20 branch September 18, 2025 20:31
@JoeCap08055 JoeCap08055 mentioned this pull request Sep 22, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants