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

feat: support for unique linking #782

Merged
merged 6 commits into from
Nov 6, 2024
Merged

feat: support for unique linking #782

merged 6 commits into from
Nov 6, 2024

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 5, 2024

Part of https://github.com/KILTprotocol/ticket/issues/3650. Built on top of #781.

Trade-off

I chose to go this way instead of providing an optional counter, because providing a counter would require one of the following two approaches:

  1. Transform the storage double map into a counted one, requiring a migration also for our currently-deployed pallet, which I wanted to avoid
  2. Not use a counter, but iterate every time to make sure there are still "spots" left for the current DID. This would require changing the benchmarking logic as now we have a potentially unbounded iteration happening. I also wanted to avoid that.

Hence, the solution was to provide a somehow more limited feature of simply specifying whether the links are expected to be unique per DID or not. This, as long as we set false for our deployed pallets would not require any storage migration, and does not require any changes in the benchmarks, so I found it a good compromise.

@ntn-x2 ntn-x2 requested a review from Ad96el November 5, 2024 08:05
// Flag specifying whether there should only ever be a single account <-> DID
// link, or multiple.
#[pallet::constant]
type UniqueLinkingEnabled: Get<bool>;
Copy link
Member Author

Choose a reason for hiding this comment

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

I made this a Get<bool> instead of a const to both allow clients to query whether this pallet enforces unique links or not, and to also make testing easier since this value can be dynamically mocked, as opposed to a const.

@ntn-x2 ntn-x2 force-pushed the aa/instantiatable-linking-pallet branch from a872cc4 to abf6587 Compare November 5, 2024 08:32
@ntn-x2 ntn-x2 force-pushed the aa/instantiatable-linking-pallet branch from abf6587 to 0514617 Compare November 5, 2024 11:01
@ntn-x2 ntn-x2 mentioned this pull request Nov 5, 2024
3 tasks
Copy link
Member

@Ad96el Ad96el left a comment

Choose a reason for hiding this comment

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

Some questions.

pallets/pallet-did-lookup/src/mock.rs Outdated Show resolved Hide resolved
pallets/pallet-did-lookup/src/lib.rs Outdated Show resolved Hide resolved
pallets/pallet-did-lookup/src/mock.rs Show resolved Hide resolved
@ntn-x2 ntn-x2 force-pushed the aa/instantiatable-linking-pallet branch from 0514617 to 84d3faf Compare November 6, 2024 07:21
Base automatically changed from aa/instantiatable-linking-pallet to develop November 6, 2024 07:22
Ad96el
Ad96el previously approved these changes Nov 6, 2024
@ntn-x2 ntn-x2 merged commit 4199b6f into develop Nov 6, 2024
2 checks passed
@ntn-x2 ntn-x2 deleted the aa/linking-limit branch November 6, 2024 08:43
ntn-x2 added a commit that referenced this pull request Nov 6, 2024
Part of KILTprotocol/ticket#3650, built on top
of #782.

I left a few comments on the review to help the reviewer understanding
the context of this changeset.

## Checklist

- [x] Add dotnames pallet to both runtimes
- [x] Add second linking deployment with unique linking to both runtimes
- [ ] ~Add new runtime API for batch resolution to both runtimes~ ->
will do in a separate PR along with a new runtime API for unique linking
resolution
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