-
Notifications
You must be signed in to change notification settings - Fork 20
Design Doc: Provider Contexts #2311
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: main
Are you sure you want to change the base?
Conversation
This provides a direct way to place image content on chain, intended to be interpreted as image content. | ||
While this must still pass through the governance step to be used by others, there is always a risk of harmful images being proposed. | ||
This risk is mitigated by: | ||
- A registered provider must be taking the action |
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 not clear from the sequence diagram, at least for the initial provider creation flow; it looks like all application data (including image bytes) would be recorded on-chain as part of the provider proposal.
Wouldn't the mitigation be submitting the image hashes rather than the images, as discussed in the "Implementation Suggestion" section?
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.
Applied some notes to highlight this.
designdocs/provider_contexts.md
Outdated
3. The `ProviderToRegistryEntry` be updated to use `ProviderToApplicationRegistryEntry` | ||
```rust | ||
#[pallet::storage] | ||
pub type ProviderToRegistryEntry<T: Config> = StorageMap< | ||
_, | ||
Twox64Concat, | ||
ProviderId, | ||
ProviderToApplicationRegistryEntry<T::MaxProviderNameSize, T::MaxProviderLogo250X100Size>, | ||
OptionQuery, | ||
>; | ||
``` | ||
4. New `ProviderToApplicationRegistryEntry` storage be initialized: | ||
```rust | ||
// Alias for clarity | ||
type ApplicationIdentifier<T: Config> = BoundedVec<u8, T::MaxProviderNameSize> | ||
|
||
#[pallet::storage] | ||
pub type ProviderToApplicationRegistryEntry<T: Config> = StorageDoubleMap< | ||
_, | ||
Twox64Concat, | ||
ProviderId, | ||
Twox64Concat, | ||
ApplicationIdentifier<T>, | ||
ApplicationRegistryEntry<T::MaxProviderNameSize, T::MaxProviderLogo250X100Size>, | ||
OptionQuery, | ||
>; | ||
``` |
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 looks confusing to me. Can a StorageMap
reference another StorageMap? Seems like there's some naming mixups.
Also, I realize that ApplicationIdentifier
is not in the critical path anywhere, but should we keep identifiers normalized (ie, numeric/integer)?
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.
🤦 Fixed. You end up with both a (required) Provider "ApplicationRegistryEntry" and an Application "ApplicationRegistryEntry" (optional). I had typo'd the storage 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.
I detect some synergy here with the concept of namespaces, as discussed in #2312 . Provider/application registration and governance flow seems like an appropriate place to put that functionality...
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.
Hmm... I'll have to think on this. It does feel like a future version of governance has a separate committee to handle this.
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 do mention in my doc, that this kind of brings up the issue of whether we should "take the plunge" on at least a partial ENS implementation, which would handle the governance and ownership aspects of names/namespaces across all pallets/contracts.
3. The `ProviderToRegistryEntry` be updated to use `ApplicationRegistryEntry` so that the Provider has a "default" `ApplicationRegistryEntry`. | ||
```rust | ||
#[pallet::storage] | ||
pub type ProviderToRegistryEntry<T: Config> = StorageMap< | ||
_, | ||
Twox64Concat, | ||
ProviderId, | ||
ApplicationRegistryEntry<T::MaxProviderNameSize, T::MaxProviderLogo250X100Size>, | ||
OptionQuery, | ||
>; | ||
``` | ||
4. New `ProviderToApplicationRegistryEntry` storage be initialized: | ||
```rust | ||
// Alias for clarity | ||
type ApplicationIdentifier<T: Config> = BoundedVec<u8, T::MaxProviderNameSize> | ||
|
||
#[pallet::storage] | ||
pub type ProviderToApplicationRegistryEntry<T: Config> = StorageDoubleMap< | ||
_, | ||
Twox64Concat, | ||
ProviderId, | ||
Twox64Concat, | ||
ApplicationIdentifier<T>, | ||
ApplicationRegistryEntry<T::MaxProviderNameSize, T::MaxProviderLogo250X100Size>, | ||
OptionQuery, | ||
>; | ||
``` |
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 appreciate the reuse of the ApplicationRegistryEntry
structure, but I think they will end up diverging (either over time, or before this document is accepted...). ProviderRegistryEntry
may end up containing more information regarding the Provider as a legal entity (similar to a DNS record), while ApplicationRegistryEntry
would not necessarily evolve in such a way, being more or less a marketing distinction drawn by the Provider.
ex:
Provider Meta
could/should reference information about the real-world ownership + "trustworthiness" of said entity.
Applications Meta.Facebook
, Meta.Instagram
, and Meta.Threads
, being simply different product offerings of the owning entity, need have nothing other than name+icon (possible other descriptive fields, but entirely at the discretion of the Provider to describe).
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 it is reasonable to at least alias or even duplicate the ApplicationRegistryEntry
to a separate ProviderRegistryEntry
. For clarity if nothing else.
I would sat that your namespacing is what ProviderToApplicationRegistryEntry
does: <ProviderId>.<ApplicationId>
That said, I'm not sure the trustworthiness can remain at the Provider level. For your example, it would be important that Meta.Threads
has the validated DNS: threads.net
and that it not apply to other Meta applications.
I am trying (and perhaps this is part of the issue here) to avoid needing to have every Provider also complete the Application data. Given that I expect <50% need both. Perhaps a better way to handle this is flipping the naming around. Rename ApplicationRegistryEntry
to ProviderRegistryEntry
. Then an ApplicationId
can "represent" themselves with a different ProviderRegistryEntry
and it is a bit more clear that Application level is an override of the default for the Provider level.
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.
If we anticipate that an Application may require a good deal of the same meta-info as a Provider for purposes of trustworthiness, then I think it's reasonable to clone/alias the info types for now.
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.
👍 Great! Thanks!
Goal
The goal of this PR is to layout a design doc for storing Provider Contexts on chain. This would allow a provider to have one or more applications and provider user consumed logos and names (in multiple languages).
Closes #2269