Skip to content
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

Schema BitFlags: Enable fine grained schema controls via optional settings #1079

Merged
merged 65 commits into from
Feb 28, 2023

Conversation

saraswatpuneet
Copy link
Collaborator

@saraswatpuneet saraswatpuneet commented Feb 13, 2023

Goal

The goal of this PR is :-

SchemaSettings: Enable fine grained schema controls via optional settings

This PR addressed updating schemas pallet and all the dependent code/tests to be updated

With @aramikm

Creates : #1106

Closes : #1020

Discussion

Checklist

  • Chain spec updated
  • Custom RPC OR Runtime API added/changed? Updated js/api-augment.
  • Design doc(s) updated
  • Tests added
  • Benchmarks added
  • Weights updated

@saraswatpuneet saraswatpuneet changed the title [WIP] Schema Grants [WIP] Schema Grants: Enable fine grained schema controls via grants Feb 13, 2023
@saraswatpuneet saraswatpuneet changed the title [WIP] Schema Grants: Enable fine grained schema controls via grants [WIP] Schema Grants: Enable fine grained schema controls via optional grants Feb 13, 2023
@saraswatpuneet saraswatpuneet marked this pull request as ready for review February 13, 2023 21:51
Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

Overall I think there is some confusion around the naming. We already have something called Grants for a schema--it is the grant of a specific permission on a schema to a delegate.

What we are adding here are schema properties or attributes, that apply to the schema as a whole, not just an individual delegate.

Maybe instead of Grant we call it a SchemaProperty?

Please let me know if I'm misunderstanding this...

@saraswatpuneet
Copy link
Collaborator Author

@JoeCap08055 when we initially worked on schemas, we said permission are nothing but schemas delegation (publishing rights) , Grants we discussed to be called to schema level controls.
We can bring it up in parking lot tomorrow :)

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.

Please see comments

})
});
weight
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

For safety reasons, a migration hook should be designed to be idempotent even if you expect to remove it before the next runtime upgrade. There should be some check like for if it's OldSchema or not before calling translate_values.

migrations::migrate_schemas_with_additional_settings::<T>()
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

try-runtime is meant to be used for testing, esp. storage migrations. If you have a look at the remove sudo storage PR it has a bunch of logging in the try-runtime, but logging is a good idea anyway in case something goes wrong.

pallets/schemas/src/tests.rs Show resolved Hide resolved
pallets/schemas/src/lib.rs Show resolved Hide resolved
// ==============================================
// END RUNTIME STORAGE MIGRATION: Schemas
// ==============================================

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shannonwells tested with this and works fine, i see schemas migrated

@@ -143,6 +150,7 @@ pub mod pallet {

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
#[pallet::storage_version(SCHEMA_STORAGE_VERSION)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Storage version will be checked at the time of migration and anything older will be translated while newer ignored

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious how this works under the hood. Just want to make sure there are no surprises

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.

Looks good! Ship it ✅

@@ -143,6 +150,7 @@ pub mod pallet {

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
#[pallet::storage_version(SCHEMA_STORAGE_VERSION)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious how this works under the hood. Just want to make sure there are no surprises

@saraswatpuneet saraswatpuneet merged commit f05867c into private-graph-main Feb 28, 2023
@saraswatpuneet saraswatpuneet deleted the schema_settings branch February 28, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change/storage-migration PR has a Storage Migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants