This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Named reserve #7778
Named reserve #7778
Changes from 35 commits
d0b265b
d13c224
597b342
47643ad
91e0b59
22e9e69
281bd90
52cb8b1
59e9b01
747c318
cd63175
e504b60
b029920
3f9a057
734363b
995dac0
0a04dce
e1644d6
1ce677e
ddfc2b0
66a2517
0301c7f
dcaf864
772258c
b107903
29f821e
b6c55e1
77ccea7
5942996
4f9154a
baf4a0f
b19dc12
72fb670
d852a81
73a0d6f
2ee5092
7614ecc
e6a1d9d
643110f
49e18f1
44d9823
28ff17f
7ba4dc1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note that despite the binary search, this is still
O(named_reservations)
(due to needing the full deserialisation ofSelf::reserves
).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.
Idea: write a wrapper for all of these defensive
unwrap_or_default
orsaturating_something
that we do that will still do the same thing, but flip an on-chain emergency button or log something.@shawntabrizi I think you once mentioned a similar idea. Did it ever make it into an issue? Else I think it can be.
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.
Could foresee having a
trait NamedReserve
or sth and then dropping thenamed
postifx, but then I would be worried that by changing an import (use traits::ReservableCurrency
->use traits::NamedReseve
) the semantics ofT::Currency::slash
would get flipped, which can lead to a pretty bad bug.But correct me if I am wrong, I think rustc is luckily not smart about this and even if only one of the traits is in scope, it would demand the universal function invocation syntax?
sth to consider overall.
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.
Good idea. I think it is possible to create a
struct NamedReserveAdpator<NamedReservableCurrency, Get<ReserveIdentifier>>
that convertsNamedReservableCurrency
intoReservableCurrency
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.
Something similar to what we have here https://github.com/open-web3-stack/open-runtime-module-library/blob/2de1e024781a7744a3aeb82229ebcb555163452a/currencies/src/lib.rs#L448
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.
probably doesnt matter, but name here is strange
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 simply add
_named
to all the methods of ReserveCurrency. Not sure if we want consistent naming convention or better method names.