-
Notifications
You must be signed in to change notification settings - Fork 24
Update provider registry and application registry via governance #2587
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
Update provider registry and application registry via governance #2587
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
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.
Overall looks good, added some nits and concerns
| // Block `create_provider`, `create_provider_v2`, `create_application` and `create_schema` calls from utility batch | ||
| RuntimeCall::Msa(pallet_msa::Call::create_provider { .. }) | | ||
| RuntimeCall::Msa(pallet_msa::Call::create_provider_v2 { .. }) | | ||
| RuntimeCall::Msa(pallet_msa::Call::create_application { .. }) | |
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 think the problem with having features on top of extrinsics was that it would modify the extrinsic indices between testnet and mainnet when we didn't have explicit index assignment for each extrinsic.
The other issue is the metadata compatibility. Having metadata for a feature that is disabled is better compared to not having any metadata for it. If we gate extrinisics by feature it would fork metadata into 2 different versions depending on if it was generated for testnet or mainnnet which can also be another pain point in future.
I think if we decide to add feature to these it should probably be inside the extrinsic body and the implementation should be refactored to support this.
| /// * [`Error::InvalidBCP47LanguageCode`] - If the provided BCP 47 language code is invalid. | ||
| /// * [`Error::ApplicationNotFound`] - If the application is not registered. | ||
| #[pallet::call_index(28)] | ||
| #[pallet::weight(T::WeightInfo::propose_to_update_application())] |
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.
nit: does this also need the same benchmark changes?
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.
Proposal does not really change the state but create a proposal and that's it so it's not useful to add or not hence all proposal_* related ones have no parameters
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.
So we use the max payload that can sent to it to compute the best worst estimate
0182963
into
feat/provider-context-development
# Details [Design Doc](https://github.com/frequency-chain/frequency/blob/main/designdocs/provider_contexts.md) Provider Context allows provider to now register a minimal profile `ProviderRegistryEntry` and `ApplicationContext` for provider's default profile and various applications providers want to register. Providers which want to register multiple applications under single provider delegation but distributed across multiple app. Following issues were resolved as part of this work with following salient points: 1. Migration existing `ProviderToRegistryEntry` to host new `ProviderRegistryEntry` payload 2. Introduce new state `ProviderToApplicationRegistry` and `ApprovedLogos` storage maps. 3. CID's are computed in deterministic way = `base(cid_v1(sha_256(logo_bytes)))` where logo_bytes are .png image bytes. 4. ApprovedLogos ensure it matches CID and logoBytes but recomputing and checking on chain. Issues complete as part of `Provider Context` work - [x] #2523 - [x] #2524 - [x] #2525 - [x] #2528 - [x] #2526 - [x] #2577 - [x] #2578 Closes #2578 Closes #2577 # Discussion items - [x] Do we feature flag than base filter out extrinsics @JoeCap08055 comment [here](#2587 (comment)) # Checklist - [x] Updated Pallet Readme? - [x] Updated js/api-augment for Custom RPC APIs? - [x] Design doc(s) updated? - [x] Unit Tests added? - [x] e2e Tests added? - [x] Benchmarks added? - [x] Spec version incremented? --------- Co-authored-by: Joe Caputo <joseph.caputo@projectliberty.io> Co-authored-by: Aramik <aramikm@gmail.com> Co-authored-by: Matthew Orris <1466844+mattheworris@users.noreply.github.com>
Goal
The goal of this PR is to introduce following 4 extrinsic to allow updating provider registry and application contexts
propose_to_update_providerupdate_provider_via_governancepropose_to_update_applicationupdate_application_via_governanceAdditionally, a
create_applicationextrinsic is introduced to make it easy to create applications for testing and is added inBaseFilterCloses #2577
Closes #2578
Checklist