Remove As trait bound from Currency::Balance #2205

xlc · 2019-04-04T11:22:26Z

(I don't expect this can be merged. This is more of an issue with related code to show my point.)

I think we should release the As bound on Balance type because it may cause issues (#1377). Even substrate code base have no such issue exists, it is impossible to ensure such issue won't exist on other runtimes.

One module requires Balance: As<usize> shouldn't affect the shared abstract Currency::Balance type otherwise it means the abstraction is not correct.
In this case the Staking module can add additional trait bound and for people are not using Staking module, they don't need to support Balance: As<usize> at all.

Another reason I want to do this is because this prevents me to use some fancy type to support multi token currency with the existing Currency trait.
I want to use Vec<(AssetId, Balance)> as the Balance type but it is impossible to convert such type to a number.

gavofyork · 2019-04-04T14:55:25Z

looks pretty reasonable, but doesn't build.

pepyakin · 2019-04-04T16:20:39Z

I want to use Vec<(AssetId, Balance)> as the Balance type but it is impossible to convert such type to a number.

Note that Copy bound on Balance can prevent you from using Vec.

xlc · 2019-04-04T21:48:15Z

I want to use Vec<(AssetId, Balance)> as the Balance type but it is impossible to convert such type to a number.

Note that Copy bound on Balance can prevent you from using Vec.

Thanks for pointing it out. I guess I need to rework the design... But be able to support Balance = (u128, u128) (due token chain) could be useful regardless.

gui1117 · 2019-04-05T09:45:17Z

srml/support/src/traits.rs

 pub trait Currency<AccountId> {
 	/// The balance of an account.
-	type Balance: SimpleArithmetic + As<usize> + As<u64> + Codec + Copy + MaybeSerializeDebug + Default;
+	type Balance: SimpleArithmetic + Codec + Copy + MaybeSerializeDebug + Default;


it seems to me that only having From<u64> and TryInto<u64> on Currency could be interesting (this would assume that a currency is overset of u64, but this is legit to me)

but SimpleArithmetic bouds As<u64> at the moment, this removal only actually removes As<usize>

Also agree with the concept of TryInto<...> (or maybe even TryFrom<...>) instead of the current one. Prevents any API break and seems like a generic-enough approach.

Question though: Balance is SimpleArithmetic and SimpleArithmetic is already As<u64>. Isn't this already indirectly implying that Balance is still bounded by As<u64>?

Question though: Balance is SimpleArithmetic and SimpleArithmetic is already As. Isn't this already indirectly implying that Balance is still bounded by As?

Yes, that's what I tried to say in my second paragraph

Right, I went blind for a sec apparently.

@xlc Did you ever test to see if this PR, with its current state, actually solve your initial problem (storing some kind of Vec as Balance)? I assume no.

No. As other pointed out, Vec is not Copyable so I need to do it differently.
I was thinking to do it with [Balance; 4] or something similar. But regardless the approach, it will likely not able to convert into a number and requires this PR.

gui1117 · 2019-04-05T09:46:38Z

I want to use Vec<(AssetId, Balance)> as the Balance type but it is impossible to convert such type to a number.

Note that Copy bound on Balance can prevent you from using Vec.

You can still wrap it in a structure and implement Copy on it though

pepyakin · 2019-04-05T10:10:51Z

You can still wrap it in a structure and implement Copy on it though

But how would you implement Copy for something that is not copy-able? One can't put Vec inside of such a structure Copy structure, one can't put something like Rc there as well.

gui1117 · 2019-04-05T10:17:05Z

You can still wrap it in a structure and implement Copy on it though

But how would you implement Copy for something that is not copy-able? One can't put Vec inside of such a structure Copy structure, one can't put something like Rc there as well.

aah you're right I thought Copy only needed Clone to be able to be marked.

xlc · 2019-04-08T22:37:40Z

I have started removing As<u64> from SimpleArithmetic and the scope is significantly more than what I initially expected.

Please let me know if you guys want me to continue on this path, or I should do it differently. Otherwise I don't want to spend any more time on a PR that is not going to get merged.

gui1117 · 2019-04-09T06:11:12Z

I have started removing As<u64> from SimpleArithmetic and the scope is significantly more than what I initially expected.

Please let me know if you guys want me to continue on this path, or I should do it differently. Otherwise I don't want to spend any more time on a PR that is not going to get merged.

I'm not sure about the way to go, removing it from simple arithmetic but bounding it to every usage of simple arithmetic won't solve the issue, does it ?

Having some kind of From<u64> equivalent in simple arithmetic is legit to me. then usages of Into<u64> should be avoided as possible and if not possible should be made through a tryinto thing.

I'm saying From and TryInto but we may prefer having our traits Sa and TryAs to be able to implement defaults as we want.

This at least make every usage of As auditable easily.

xlc · 2019-04-09T06:48:10Z

I have started removing As<u64> from SimpleArithmetic and the scope is significantly more than what I initially expected.
Please let me know if you guys want me to continue on this path, or I should do it differently. Otherwise I don't want to spend any more time on a PR that is not going to get merged.

I'm not sure about the way to go, removing it from simple arithmetic but bounding it to every usage of simple arithmetic won't solve the issue, does it ?

Having some kind of From<u64> equivalent in simple arithmetic is legit to me. then usages of Into<u64> should be avoided as possible and if not possible should be made through a tryinto thing.

I'm saying From and TryInto but we may prefer having our traits Sa and TryAs to be able to implement defaults as we want.

This at least make every usage of As auditable easily.

It does make a difference. Only the Currency::Balance need to remove As<u64> bound. For the module needs it, e.g. staking, it can still require the As bound and I can choose either not using staking module, or pass a specialized trait implementation confirms the As in a meaningful way in the context of staking module. For a different module, I have the chose to pass a different trait implementation.

In the context of the module we are working on, a multi token module (similar to assets module in substrate), I can pass the main token Currency struct to staking module, pass the generalized Currency struct to another module with does not have As bound.

So maybe keep SimpleArithmetic as it is and have another version of it without As<u64> and use it for Currency::Balance?

gavofyork · 2019-04-16T07:52:19Z

In general I would like to see As removed entirely and switch it to TryFrom<u64>. There should be no need for TryInto<u64>; for modules that appear to need to convert a SimpleArithmetic into a u64, then there should probably be an explicit conversion type (i.e. a type of trait Convert<...> passed in to the module.

gui1117 · 2019-04-17T20:25:25Z

Currency bounds to TryFrom<u64>, it would fail for which reason ? if value is subset of u64, or if value doesn't make sense to be converted from u64 and doesn't want to support this ?

if the latter then it seems that there is some contradiction: because it seems I can reimplement From using Shl and One.

EDIT: or with One and Add

gui1117 · 2019-04-17T20:55:01Z

(also it seems a lot of conertion are made between u64 and BlockNumber in core)

(EDIT: this is not an argument for or against)

kianenigma · 2019-04-19T12:27:40Z

I suggest this PR (if it is not to be continued as-is) to be merged (close one and combine work in one) with #2322 (cc @thiolliere). They both seem to be experimenting toward one goal.

xlc · 2019-04-21T12:30:40Z

I have reverted the attempt to completely remove As from Currency::Balance type as it depends on #2322

The rest of the PR is still a cleanup of the code and can be merged.

gui1117

if this PR is about some code refactor then this comments are relevant now.

srml/staking/src/lib.rs

xlc · 2019-05-08T03:44:43Z

@thiolliere updated

gui1117

So what this PR does is just removing As<usize> from balance associated type. As<u64> is still bound by SimpleArithmetic

gui1117 · 2019-05-08T07:31:50Z

srml/staking/src/lib.rs

-			let total_minted = reward * <BalanceOf<T> as As<usize>>::sa(validators.len());
-			let total_rewarded_stake = Self::slot_stake() * <BalanceOf<T> as As<usize>>::sa(validators.len());
+			let len = validators.len() as u64; // validators length can never overflow u64
+			let len = BalanceOf::<T>::sa(len);


I would have written this in one line but whatever

* remove As<64> bond * Currency trait refactor * bump spec version

xlc changed the title ~~Update balance~~ Remove As trait bound from Currency::Balance Apr 4, 2019

devops-parity added the A4-gotissues label Apr 4, 2019

xlc mentioned this pull request Apr 4, 2019

code cleanup #2206

Merged

xlc force-pushed the update-balance branch from b1af445 to 3d32968 Compare April 4, 2019 21:39

gui1117 suggested changes Apr 5, 2019

View reviewed changes

xlc force-pushed the update-balance branch from 51ed982 to ac3f5a9 Compare April 21, 2019 12:27

xlc force-pushed the update-balance branch 2 times, most recently from ac3f5a9 to dc8787c Compare April 21, 2019 12:35

devops-parity added the B2-breaksapi label Apr 21, 2019

gui1117 suggested changes Apr 21, 2019

View reviewed changes

srml/staking/src/lib.rs Outdated Show resolved Hide resolved

srml/staking/src/lib.rs Show resolved Hide resolved

gui1117 mentioned this pull request Apr 23, 2019

Review occurences of UniqueSaturated[From, Into] trait usage for potential lossy casts #1377

Closed

kianenigma mentioned this pull request May 7, 2019

The mul operation of Perbill doesn't work properly #2441

Closed

remove As<64> bond

2fd395b

xlc force-pushed the update-balance branch from dc8787c to 7a28703 Compare May 8, 2019 02:31

Currency trait refactor

dc39843

xlc force-pushed the update-balance branch from 7a28703 to dc39843 Compare May 8, 2019 02:44

bump spec version

01b2d47

gui1117 self-requested a review May 8, 2019 07:31

gui1117 approved these changes May 8, 2019

View reviewed changes

Merge branch 'master' into update-balance

7bc768c

gavofyork approved these changes May 9, 2019

View reviewed changes

gavofyork merged commit 3b6dba5 into paritytech:master May 9, 2019

xlc deleted the update-balance branch May 9, 2019 07:55

xlc mentioned this pull request May 31, 2019

Smart Contract to support Multi Token cennznet/cennznet#7

Closed

MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019

Remove As trait bound from Currency::Balance (paritytech#2205)

68f9b6b

* remove As<64> bond * Currency trait refactor * bump spec version

Remove As trait bound from Currency::Balance #2205

Remove As trait bound from Currency::Balance #2205

Uh oh!

Conversation

xlc commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavofyork commented Apr 4, 2019

Uh oh!

pepyakin commented Apr 4, 2019

Uh oh!

xlc commented Apr 4, 2019

Uh oh!

gui1117 Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

kianenigma Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

gui1117 Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

kianenigma Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

xlc Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepyakin commented Apr 5, 2019

Uh oh!

gui1117 commented Apr 5, 2019

Uh oh!

xlc commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gui1117 commented Apr 9, 2019

Uh oh!

xlc commented Apr 9, 2019

Uh oh!

gavofyork commented Apr 16, 2019

Uh oh!

gui1117 commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gui1117 commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kianenigma commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xlc commented Apr 21, 2019

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xlc commented May 8, 2019

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

gui1117 May 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xlc commented Apr 4, 2019 •

edited

Loading

xlc Apr 8, 2019 •

edited

Loading

gui1117 commented Apr 5, 2019 •

edited

Loading

xlc commented Apr 8, 2019 •

edited

Loading

gui1117 commented Apr 17, 2019 •

edited

Loading

gui1117 commented Apr 17, 2019 •

edited

Loading

kianenigma commented Apr 19, 2019 •

edited

Loading