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

Implement NamedMultiReservableCurrency and NamedBasicReservableCurrency in orml_currencies #743

Merged

Conversation

Chralt98
Copy link
Contributor

Hey, this is related to #742.

It's the first try and it's not ready yet.

@xlc
Copy link
Member

xlc commented May 17, 2022

Ok. Simply add the trait bound makes no sense (that's what I read from the issue) but actually implement NamedMultiReservableCurrency is useful.

@Chralt98
Copy link
Contributor Author

@xlc I don't get why the cargo fmt isn't correct. I formatted it multiple times.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #743 (8e4cd47) into master (39057da) will decrease coverage by 0.51%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #743      +/-   ##
==========================================
- Coverage   77.68%   77.17%   -0.52%     
==========================================
  Files          92       92              
  Lines        9069     9129      +60     
==========================================
  Hits         7045     7045              
- Misses       2024     2084      +60     
Impacted Files Coverage Δ
currencies/src/lib.rs 42.21% <0.00%> (-8.78%) ⬇️
traits/src/currency.rs 34.88% <0.00%> (-25.12%) ⬇️
traits/src/lib.rs 0.00% <ø> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Chralt98
Copy link
Contributor Author

I let you finish the work. I think the most is done.

@xlc xlc requested review from shaunxw and zqhxuyuan May 18, 2022 22:30
@Chralt98
Copy link
Contributor Author

Chralt98 commented May 19, 2022

I don't really know if this might be a problem in the future. I mean we got a ReserveIdentifier for orml_tokens AND another ReserveIdentifier for pallet_balances.

PS: I hate this cargo fmt problem here in this repo...
Needed to use cargo +nightly fmt here.

@Chralt98 Chralt98 requested review from zqhxuyuan and xlc May 19, 2022 10:41
@Chralt98 Chralt98 changed the title Add NamedMultiReservableCurrency in orml_currencies Implement NamedMultiReservableCurrency and NamedBasicReservableCurrency in orml_currencies May 19, 2022
@Chralt98 Chralt98 marked this pull request as ready for review May 19, 2022 11:38
@xlc
Copy link
Member

xlc commented May 19, 2022

Update your cargo to a recent version should resolve the fmt issue. This is one used by CI

toolchain: nightly-2022-02-19

@xlc
Copy link
Member

xlc commented May 19, 2022

Multiple ReserveIdentifier is not an issue. You can require them to be the same one. Similar to how the Balance type for pallet-balances and orml-tokens are handled.

@Chralt98
Copy link
Contributor Author

Thanks @xlc ! If there is anything to correct in my pull request, I am open to correct it after a review.

@Chralt98 Chralt98 requested a review from xlc May 23, 2022 07:15
@zqhxuyuan
Copy link
Contributor

LGTM except missing testcase, can you do this here or maybe in another PR? @Chralt98

@Chralt98
Copy link
Contributor Author

@zqhxuyuan This depends on, what would be faster for a merge (Zeitgeist requires this functionality as fast as possible). I would propose, that it should get another pull request. But please verify the code before merge again :-D. I don't like to impact many chains to loose money. I would try to add test cases later in the day in another pull request. But I would be happy, if you could spend time on this too.

Copy link
Contributor

@zqhxuyuan zqhxuyuan left a comment

Choose a reason for hiding this comment

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

LGTM

@zqhxuyuan zqhxuyuan merged commit 52ec52b into open-web3-stack:master May 23, 2022
@zqhxuyuan
Copy link
Contributor

It's better to add can_reserve method too, you can do this on later PR. @Chralt98

@Chralt98
Copy link
Contributor Author

Chralt98 commented May 23, 2022

@zqhxuyuan You mean can_reserve_named for NamedBasicReservableCurrency and for NamedMultiReservableCurrency then? But this is not implemented for orml_tokens. Or what do you mean? And it's not part of the NamedReservableCurrency by substrate either.

@zqhxuyuan
Copy link
Contributor

ok, my bad. the can_reseve is not needed for named implementation.

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.

4 participants