-
Notifications
You must be signed in to change notification settings - Fork 47
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
chore: instantiatable linking pallet #781
Conversation
abf6587
to
0514617
Compare
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.
LGTM
where | ||
T::AccountId: Into<LinkableAccountId> + From<sp_runtime::AccountId32> + Into<sp_runtime::AccountId32>, | ||
<T as frame_system::Config>::AccountId: |
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.
Why is this change necessary?
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 slipped in. Not a big deal tho.
0514617
to
84d3faf
Compare
Part of KILTprotocol/ticket#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.
Part of https://github.com/KILTprotocol/ticket/issues/3650. Built on top of #779.