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

TRN-808 Sylo Data Pallet #917

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

TRN-808 Sylo Data Pallet #917

wants to merge 20 commits into from

Conversation

JCSanPedro
Copy link
Collaborator

Description

The PR implements and adds the Sylo Data Verification Pallet to the runtime.

Protocol RFC - https://www.notion.so/futureverse/RFC-Sylo-Data-Verification-Protocol-14a0cb4dab3d808389eddaab37a2c767

The pallet allows users to register and manage resolvers, and also create and manage validation records.

Resolvers

A resolver is intended to be an external service capable of storing and providing off-chain data. The pallet allows a user to register a resolver, which would include an identifier and a list of service endpoints. A resolver is identified by a ResolverId, which loosely follows the DID Specification.

The ResolverId is comprised of two components: a method and identifier. The method describes what kind of resolver it is. All resolvers registered on chain are considered "sylo" resolvers, and will all use the same reserved string for their resolver method.

The account which registers the resolver is considered the controller of the register, and is capable of modifying or removing the resolver later.

Validation Records

The pallet allows a user to create a validation record through the create_validation_record extrinsic. A validation record references an externally stored piece of data, and is intended to be used to validate the integrity and authenticity of offchain data.

A validation record will have the following attributes:

  • data_id - A string representing the id. Validation records are stored in a double map, using the data_id as well as the author's account id.
  • author - The account which created the record. Will have the ability to modify and or delete the record in the future.
  • resolvers - A list of ResolverIds which reference how to fetch the data. All resolvers using the reserved sylo method must be registered onchain.
  • data_type - A string acting as a hint to clients as to what type of data it is.
  • tags - A list of strings acting as additional metadata information
  • entries A list of validation record entry. Each entry represents a distinct version of the data. It includes a checksum for integrity, and also a block number for when the entry was created.

Notes

  • Payment - All extrinsics related to managing validation records and resolvers are paid using Sylo tokens. This is achieved by modifying the OnChargeTransaction implementation for the FeeProxy, to detect sylo related extrinsics, and force the swap.

Release Notes

Key Changes

Adds the Sylo Data Verification Pallet to the runtime

Type of Change

  • Runtime Changes

Extrinsic Changes

Added

  • Sylo: register_resolver
  • Sylo: update_resolver
  • Sylo: unregister_resolver
  • Sylo: create_validation_record
  • Sylo: add_validation_record_entry
  • Sylo: update_validation_record
  • Sylo: delete_validation_record

Comment on lines 600 to 604
pub const MaxResolvers: u32 = 10;
pub const MaxTags: u32 = 10;
pub const MaxEntries: u32 = 100;
pub const MaxServiceEndpoints: u32 = 10;
pub const SyloStringLimit: u32 = 500;
Copy link
Contributor

Choose a reason for hiding this comment

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

could reduce space for these vars:

Suggested change
pub const MaxResolvers: u32 = 10;
pub const MaxTags: u32 = 10;
pub const MaxEntries: u32 = 100;
pub const MaxServiceEndpoints: u32 = 10;
pub const SyloStringLimit: u32 = 500;
pub const MaxResolvers: u8 = 10;
pub const MaxTags: u8 = 10;
pub const MaxEntries: u8 = 100;
pub const MaxServiceEndpoints: u8 = 10;
pub const SyloStringLimit: u16 = 500;

Copy link
Contributor

Choose a reason for hiding this comment

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

weights need to be generated via CI - the runner would commit the updated weights

@@ -723,3 +723,26 @@ macro_rules! impl_pallet_scheduler_config {
}
};
}

#[macro_export]
macro_rules! impl_pallet_sylo_config {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice; thanks for this

@@ -23,6 +23,7 @@ pallet-evm = { workspace = true }
pallet-assets-ext = { workspace = true }
pallet-dex = { workspace = true }
pallet-futurepass = { workspace = true }
pallet-sylo = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

may need an entry in the std = [ feature below.

Copy link
Contributor

Choose a reason for hiding this comment

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

will wait for validated E2E tests to approve changes here

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we need explicit/restrictive trait-bounds for all the types.
Can take a look at the pub struct SaleInformation<AccountId, BlockNumber> { in pallet/crowdsale/src/types.rs file to see struct declarations which just consume a generic.

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 tried something like

pub struct ResolverId<Str> {
	pub method: Str,
	pub identifier: Str,
}

pub trait ToDid {
	fn to_did(&self) -> Vec<u8>;
}

impl<T: Get<u32>> ToDid for ResolverId<BoundedVec<u8, T>> {
..
}

but didn't feel like it cleaned up the code, as in the lib implementation, I then had to reference the full type like this ResolverId<BoundedVec<u8, T::StringLimit>> everywhere

#[pallet::weight({
T::WeightInfo::set_payment_asset()
})]
pub fn set_payment_asset(origin: OriginFor<T>, payment_asset: AssetId) -> DispatchResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

event needs to be emitted for all state changes.

#[pallet::weight({
T::WeightInfo::set_sylo_resolver_method()
})]
pub fn set_sylo_resolver_method(
Copy link
Contributor

Choose a reason for hiding this comment

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

same here (emit event)

T::WeightInfo::set_payment_asset()
})]
pub fn set_payment_asset(origin: OriginFor<T>, payment_asset: AssetId) -> DispatchResult {
ensure_root(origin)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than using root; use the ApproveOrigin account (passed in via runtime)

origin: OriginFor<T>,
resolver_method: BoundedVec<u8, T::StringLimit>,
) -> DispatchResult {
ensure_root(origin)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here (use approved origin)

Comment on lines 253 to 268
let mut resolver =
<Resolvers<T>>::get(identifier.clone()).ok_or(Error::<T>::ResolverNotRegistered)?;

ensure!(who == resolver.controller, Error::<T>::NotController);

resolver.service_endpoints = service_endpoints.clone();

<Resolvers<T>>::insert(identifier.clone(), resolver);

Self::deposit_event(Event::ResolverUpdated {
id: identifier.to_vec(),
controller: who,
service_endpoints,
});

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: instead of getting and inserting:

Suggested change
let mut resolver =
<Resolvers<T>>::get(identifier.clone()).ok_or(Error::<T>::ResolverNotRegistered)?;
ensure!(who == resolver.controller, Error::<T>::NotController);
resolver.service_endpoints = service_endpoints.clone();
<Resolvers<T>>::insert(identifier.clone(), resolver);
Self::deposit_event(Event::ResolverUpdated {
id: identifier.to_vec(),
controller: who,
service_endpoints,
});
Ok(())
<Resolvers<T>>::try_mutate(&identifier, |maybe_resolver| {
let resolver = maybe_resolver.as_mut().ok_or(Error::<T>::ResolverNotRegistered)?;
ensure!(who == resolver.controller, Error::<T>::NotController);
resolver.service_endpoints = service_endpoints.clone();
Self::deposit_event(Event::ResolverUpdated {
id: identifier.into_vec(),
controller: who,
service_endpoints,
});
Ok(())
})

Comment on lines 281 to 286
let resolver =
<Resolvers<T>>::get(identifier.clone()).ok_or(Error::<T>::ResolverNotRegistered)?;

ensure!(who == resolver.controller, Error::<T>::NotController);

<Resolvers<T>>::remove(identifier.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary clone (memory allocations) can be removed i think

Suggested change
let resolver =
<Resolvers<T>>::get(identifier.clone()).ok_or(Error::<T>::ResolverNotRegistered)?;
ensure!(who == resolver.controller, Error::<T>::NotController);
<Resolvers<T>>::remove(identifier.clone());
let resolver = <Resolvers<T>>::get(&identifier).ok_or(Error::<T>::ResolverNotRegistered)?;
ensure!(who == resolver.controller, Error::<T>::NotController);
<Resolvers<T>>::remove(&identifier);

let who = ensure_signed(origin)?;

ensure!(
<ValidationRecords<T>>::get(who.clone(), data_id.clone()).is_none(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ValidationRecords<T>>::get(who.clone(), data_id.clone()).is_none(),
<ValidationRecords<T>>::get(&who, &data_id).is_none(),

Comment on lines 437 to 453
pub fn validate_sylo_resolvers(
resolvers: BoundedVec<ResolverId<T::StringLimit>, T::MaxResolvers>,
) -> DispatchResult {
let reserved_method = <SyloResolverMethod<T>>::get();

// Ensure any sylo data resolvers are already registered
for resolver in resolvers {
if resolver.method == reserved_method {
ensure!(
<Resolvers<T>>::get(resolver.identifier).is_some(),
Error::<T>::ResolverNotRegistered
);
}
}

Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
pub fn validate_sylo_resolvers(
resolvers: BoundedVec<ResolverId<T::StringLimit>, T::MaxResolvers>,
) -> DispatchResult {
let reserved_method = <SyloResolverMethod<T>>::get();
// Ensure any sylo data resolvers are already registered
for resolver in resolvers {
if resolver.method == reserved_method {
ensure!(
<Resolvers<T>>::get(resolver.identifier).is_some(),
Error::<T>::ResolverNotRegistered
);
}
}
Ok(())
}
pub fn validate_sylo_resolvers(
resolvers: &BoundedVec<ResolverId<T::StringLimit>, T::MaxResolvers>,
) -> DispatchResult {
let reserved_method = <SyloResolverMethod<T>>::get();
resolvers
.iter()
.filter(|resolver| resolver.method == reserved_method)
.try_for_each(|resolver| {
ensure!(
<Resolvers<T>>::contains_key(&resolver.identifier),
Error::<T>::ResolverNotRegistered
);
Ok(())
})
}

Error::<T>::RecordAlreadyCreated
);

Self::validate_sylo_resolvers(resolvers.clone())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Self::validate_sylo_resolvers(resolvers.clone())?;
Self::validate_sylo_resolvers(&resolvers)?;

}]),
};

<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);
<ValidationRecords<T>>::insert(&who, &data_id, record);

) -> DispatchResult {
let who = ensure_signed(origin)?;

let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
let mut record = <ValidationRecords<T>>::get(&who, &data_id)

Comment on lines 348 to 364
let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
.ok_or(Error::<T>::RecordNotCreated)?;

record.entries.force_push(ValidationEntry {
checksum: checksum.clone(),
block: <frame_system::Pallet<T>>::block_number(),
});

<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);

Self::deposit_event(Event::ValidationEntryAdded {
author: who,
id: data_id.to_vec(),
checksum,
});

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
.ok_or(Error::<T>::RecordNotCreated)?;
record.entries.force_push(ValidationEntry {
checksum: checksum.clone(),
block: <frame_system::Pallet<T>>::block_number(),
});
<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);
Self::deposit_event(Event::ValidationEntryAdded {
author: who,
id: data_id.to_vec(),
checksum,
});
Ok(())
<ValidationRecords<T>>::try_mutate(&who, &data_id, |maybe_record| {
let record = maybe_record.as_mut().ok_or(Error::<T>::RecordNotCreated)?;
record.entries.force_push(ValidationEntry {
checksum,
block: <frame_system::Pallet<T>>::block_number(),
})?;
Self::deposit_event(Event::ValidationEntryAdded {
author: who,
id: data_id.into_vec(),
checksum,
});
Ok(())
})

Comment on lines 380 to 407
let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
.ok_or(Error::<T>::RecordNotCreated)?;

if let Some(resolvers) = resolvers.clone() {
Self::validate_sylo_resolvers(resolvers.clone())?;
record.resolvers = resolvers;
}

if let Some(data_type) = data_type.clone() {
record.data_type = data_type;
}

if let Some(tags) = tags.clone() {
record.tags = tags;
}

<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);

Self::deposit_event(Event::ValidationRecordUpdated {
author: who,
id: data_id.to_vec(),
resolvers: resolvers
.map(|resolvers| resolvers.iter().map(|resolver| resolver.to_did()).collect()),
data_type: data_type.map(|data_type| data_type.to_vec()),
tags: tags.map(|tags| tags.iter().map(|tag| tag.to_vec()).collect()),
});

Ok(())
Copy link
Contributor

@zees-dev zees-dev Jan 8, 2025

Choose a reason for hiding this comment

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

more efficient as we only clone if/when required:

Suggested change
let mut record = <ValidationRecords<T>>::get(who.clone(), data_id.clone())
.ok_or(Error::<T>::RecordNotCreated)?;
if let Some(resolvers) = resolvers.clone() {
Self::validate_sylo_resolvers(resolvers.clone())?;
record.resolvers = resolvers;
}
if let Some(data_type) = data_type.clone() {
record.data_type = data_type;
}
if let Some(tags) = tags.clone() {
record.tags = tags;
}
<ValidationRecords<T>>::insert(who.clone(), data_id.clone(), record);
Self::deposit_event(Event::ValidationRecordUpdated {
author: who,
id: data_id.to_vec(),
resolvers: resolvers
.map(|resolvers| resolvers.iter().map(|resolver| resolver.to_did()).collect()),
data_type: data_type.map(|data_type| data_type.to_vec()),
tags: tags.map(|tags| tags.iter().map(|tag| tag.to_vec()).collect()),
});
Ok(())
<ValidationRecords<T>>::try_mutate(&who, &data_id, |maybe_record| {
let record = maybe_record.as_mut().ok_or(Error::<T>::RecordNotCreated)?;
if let Some(ref new_resolvers) = resolvers {
Self::validate_sylo_resolvers(new_resolvers)?;
record.resolvers = new_resolvers.clone();
}
if let Some(ref new_data_type) = data_type {
record.data_type = new_data_type.clone();
}
if let Some(ref new_tags) = tags {
record.tags = new_tags.clone();
}
Self::deposit_event(Event::ValidationRecordUpdated {
author: who,
id: data_id.into_vec(),
resolvers: resolvers.map(|r| r.iter().map(|resolver| resolver.to_did()).collect()),
data_type: data_type.map(|dt| dt.into_vec()),
tags: tags.map(|t| t.iter().map(|tag| tag.into_vec()).collect()),
});
Ok(())
})

let who = ensure_signed(origin)?;

ensure!(
<ValidationRecords<T>>::get(who.clone(), data_id.clone()).is_some(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ValidationRecords<T>>::get(who.clone(), data_id.clone()).is_some(),
<ValidationRecords<T>>::contains_key(&who, &data_id),

Error::<T>::RecordNotCreated
);

<ValidationRecords<T>>::remove(who.clone(), data_id.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ValidationRecords<T>>::remove(who.clone(), data_id.clone());
<ValidationRecords<T>>::remove(&who, &data_id);

Ok(())
}

pub fn payment_asset() -> Option<AssetId> {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems redundant; can just call the type function directly IMO


let mut service_endpoints = BoundedVec::new();
for _ in 1..q {
service_endpoints.force_push(bounded_string::<T>("https://service-endpoint.one.two.three"));
Copy link
Contributor

Choose a reason for hiding this comment

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

probaby push max string size here too; we need upper bound test

for _ in 1..q {
service_endpoints.force_push(bounded_string::<T>("https://service-endpoint.one.two.three"));
}
}: _(origin::<T>(&alice), identifier, service_endpoints)
Copy link
Contributor

@zees-dev zees-dev Jan 8, 2025

Choose a reason for hiding this comment

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

worthwhile having benchmark validations; you can look at other benchmarks (maybe crowdsale pallet) - which has examples of quick validations post-test.
A single validation should do.

type WeightInfo = ();
}

#[derive(Default)]
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should be able to use the test helpers testext instead od declaring a new/custom one here

}
}

fn create_and_register_resolver(
Copy link
Contributor

Choose a reason for hiding this comment

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

move helper functions to top; i think we follow this approach for most of the tests

return (data_id, resolvers, data_type, tags, checksum, record);
}

fn create_resolver_id(method: &str, identifier: &str) -> ResolverId<<Test as Config>::StringLimit> {
Copy link
Contributor

Choose a reason for hiding this comment

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

dont think this is needed; can inline into the caller function

}
}

mod validation_records {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ideally we try create a mod for each extrinsic - which is easier to reason as we can test all flows within the same module.
Examples of this should be in the crowdsale or nft pallet tests.

Copy link
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

nice; looking mostly good!
Most of the suggestions are purely syntactical improvements and/or improvements to memory.

Atleast 2 major things are still required:

  • benchmark weight updates
  • e2e tests to validate sylo token payment (NOTE: the refund for any over-payment would be given in the native currency)

Other than that; logic seems sound.

@JCSanPedro JCSanPedro force-pushed the TRN-808-sylo-pallet branch 2 times, most recently from 5a325b7 to 7ed93e4 Compare January 12, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants