-
Notifications
You must be signed in to change notification settings - Fork 20
feat: implement pallet_msa::withdraw_tokens extrinsic #2402
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
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
- address some PR comments - make extrinsic free - update tests for free extrinsic
pallets/msa/src/lib.rs
Outdated
@@ -1501,7 +1595,7 @@ impl<T: Config> Pallet<T> { | |||
} | |||
|
|||
// Add the newest signature if we are not the first | |||
if pointer.count != 0 { | |||
if pointer.count != 0 && current_block.le(&pointer.newest_expires_at) { |
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.
@wilwade while examining the signature registry methods in order to split checking (read) from recording (read/write) signatures, I came up with this tiny optimization--we don't need to add the previously-most-recent signature to the larger list if it's already expired.
Double-check me on that?
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.
Yes. It took me a bit, but it works. Added a change to the comment above it to make it clear what it is doing, but that is optional.
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 I take this back.
It doesn't work and apparently we don't have a test for it. I think...
It does work. See next comment.
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.
For others reading this, this might help:
The comment on the top of register_signature
says this:
1,2 (oldest)
2,3
3,4
4 (pointer storage)
I thought Joe's idea is that when 5 comes in, if 4 is ALREADY expired, instead of adding 4,5
and 5 (pointer storage)
we ONLY need to keep 5 (pointer storage)
However, this "breaks the loop" So does it work or not?
The answer is yes. Because: The comment is wrong/misleading. It should be
/// Example list:
/// - `1,2 (oldest)`
/// - `2,3`
/// - `3,4` (newest, is only in pointer storage)`
So Joe's actual idea is that when 5 comes in, if 4 is ALREADY expired, instead of adding 3,4
and 4,5 (pointer storage)
we ONLY update the pointer storage to 3,5 (pointer storage)
That keeps the loop intact.
So @JoeCap08055 you are good to make the change, but please update the comments a bit to make it more clear (and fix my bad original commenting)
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.
@wilwade (and maybe this is a bit out of scope here, but since we're messing with this algorithm anyway...) I notice that the signature storage, once it fills up, will from that point on always contain the max # of entries, even once they've all expired. Have we considered the comparative costs of maintaining that steady-state storage for all time, versus opportunistically pruning any time a new signature comes in?
(I understand that the pruning cost could end up being expensive for a single "free" transaction if we pruned all expired entries. Possibly we could strike a balance, say... any time a new signature comes in, prune the N
oldest instead of just the oldest, which would lead to a, if not empty, at least lower steady-state storage cost over time...)
If this seems of interest I could write up an issue if we don't want to include here...
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.
(Also, I'm going to revert my change: since, currently, storage will always => max entries, and in order to make my change work, it still needs to perform a write (update instead of insert, but same cost), there seems no need to mess with this method unless we're going to address my comment above.)
Co-authored-by: Matthew Orris <[email protected]>
Goal
The goal of this PR is to implement an initia
withdraw_tokens
extrinsic that allows MSAs holding tokens to authorize their tokens to be withdrawn to a specified account.Closes #2388
Discussion
Checklist