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

pallets: Add Anchors V2 pallet #1878

Merged
merged 12 commits into from
Jun 24, 2024
Merged

pallets: Add Anchors V2 pallet #1878

merged 12 commits into from
Jun 24, 2024

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented Jun 18, 2024

Description

Adds a new anchors pallet, required for the data protocol.

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

I think the logic is perfect! 💯 Very clean and easy to follow.

Some NITs, questions and suggestions, nothing severe

pallets/anchors-v2/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
pallets/anchors-v2/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 96 to 100
/// Storage for document anchors.
#[pallet::storage]
#[pallet::getter(fn get_document_anchor)]
pub type DocumentAnchors<T: Config> =
StorageMap<_, Blake2_256, (DocumentId, DocumentVersion), T::AnchorIdNonce>;
Copy link
Contributor

@lemunozm lemunozm Jun 20, 2024

Choose a reason for hiding this comment

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

Is there a 1:1 relation among (DocumentId, DocumentVersion) <> T::AnchorIdNOnce?, Could it be the own (DocumentId, DocumentVersion) the id of the anchor id itself?

That way in this case, you do not need an AnchorIdNOnce

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but then storing personal anchors might be problematic, or we can just use a double map and store (). Lemme try that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it! Super simple now! 💯

@cdamian cdamian force-pushed the feature/add-anchors-v2-pallet branch 2 times, most recently from fcfdf33 to 179570b Compare June 20, 2024 20:13
@cdamian
Copy link
Contributor Author

cdamian commented Jun 20, 2024

Not sure why the fmt lint is acting up, I don't get the same errors locally... Re-installing the toolchain fixed that, weird.

@cdamian cdamian force-pushed the feature/add-anchors-v2-pallet branch 4 times, most recently from 7b6dd59 to d4d20e3 Compare June 20, 2024 20:45
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM @cdamian! Good job with this pallet, everything done in less than 1K lines, super clean! 💯

@@ -21,7 +21,7 @@ use super::*;

#[benchmarks(
where
T: Config<Balance = u128, Hash = H256, AnchorIdNonce = u128>,
T: Config<Balance = u128, Hash = H256>,
Copy link
Contributor

@lemunozm lemunozm Jun 24, 2024

Choose a reason for hiding this comment

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

This is not bad, and probably this pallet is only for us, so it will not be an issue. But to explain the why:

These bounds force the user of this pallet to always choose Balance = u128 and Hash = H256 in their runtime if they want to perform the benchmarks. Ideally, we do not want to restrict the usage of the bounds to certain concrete types. We want to allow any type that can be configured by the pallet.

The solution for that is to add extra bounds to those associated types as:

T: Config,
T::Balance: Into<u128>, // Or maybe T::Balance: Default is just enough
T::Hash: Default, // And use later T::Hash::default() instead of H256::from_low_u64_be(1)

Not blocker at all, just for reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can leave it as it is for now, these benchmarks are still tests at the end of the day and I think it's fine to have some hardcoded types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not blocker 👍🏻

@cdamian cdamian force-pushed the feature/add-anchors-v2-pallet branch from 77c0bc9 to bf3d2e4 Compare June 24, 2024 08:10
@cdamian cdamian marked this pull request as ready for review June 24, 2024 08:11
@cdamian cdamian merged commit 737278c into main Jun 24, 2024
10 checks passed
@cdamian cdamian deleted the feature/add-anchors-v2-pallet branch June 24, 2024 09:48
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.

2 participants