Fees in community currency #190

pifragile · 2022-03-30T13:12:06Z

Fot the node integration see encointer/encointer-node#194

clangenb · 2022-03-31T14:39:54Z

balances/src/impl_fungibles.rs

+	fn name(asset: &Self::AssetId) -> Vec<u8> {
+		PalletString::from("Encointer").into()
+	}
+
+	fn symbol(asset: &Self::AssetId) -> Vec<u8> {
+		PalletString::from("ETR").into()
+	}


These should be community currency-specific entries and nod global ones. But this is only information afaik, so it should be fine in our case.

clangenb · 2022-03-31T14:53:09Z

balances/src/impl_fungibles.rs

@@ -0,0 +1,180 @@
+use super::*;


You left quite a few implementations out compared to the one in assets: https://github.com/paritytech/substrate/blob/master/frame/assets/src/impl_fungibles.rs

On what criteria did you decide? I am asking just out of curiosity. :)

i just tried to implement the smallest subset necessary for our usecase :)

clangenb · 2022-03-31T15:55:11Z

communities/src/lib.rs

@@ -696,6 +701,83 @@ impl<T: Config> Pallet<T> {
 	}
 }

+pub type OnChargeTransactionOf<T> = <T as pallet_transaction_payment::Config>::OnChargeTransaction;


Why do you define this one here and not in the encointer-balances pallet?

Ok, I see because of the dependency stuff, hence I propose to define this thing in a separate crate.

I moved the stuff to the encointer-balances-tx-payment crate

communities/Cargo.toml

balances/src/lib.rs

clangenb · 2022-03-31T16:10:16Z

balances/src/impl_fungibles.rs

+				((fungible_balance << 64) >> 64) as f64 / (10i128.pow(decimals as u32) as f64),
+			);


((fungible_balance << 64) >> 64) I guess this is to truncate, however we might find some higher level function for this. :)

This is actually the same as fungible_balance as u64

balances/src/impl_fungibles.rs

balances-tx-payment-extension/src/balance_conversion.rs

…actor

clangenb · 2022-04-01T18:03:49Z

primitives/src/balances.rs

+		let mut result: BalanceType = BalanceType::from_num(0);
+
+		result += BalanceType::from_num(
+			((fungible_balance << 64) >> 64) as f64 / (10i128.pow(decimals as u32) as f64),


These conversions might happen in the runtime. You must never use indeterministic data types there.

Changed to using fixed point types.

…ives

clangenb

Although I had quite a few comments, it looked good in general. I just resolved all the remarks myself due to time-constraints. Don't take it personally. :)

Just a few notes:

never use floating-point types by stuff that is executed in the runtime. This is not deterministic and will lead to consensus errors.
you sometimes use quite crazy math-ops. Although they might be very elegant, they are hard to follow. Some more doc there would be nice.

Ready to merge from my point of view. It compiles with the node.

balances-tx-payment/src/balance_conversion.rs

…to be adjusted more freely

…ee_conversion_factor`

brenzi · 2022-04-02T15:58:15Z

balances-tx-payment/src/tests.rs

 use rstest::*;

-/// 1 micro KSM with 12 decimals
-const ONE_MICRO_KSM: u128 = 1_000_000;
-
 /// 1 community currency token with 18 decimals
 const ONE_CC: u128 = 1_000_000_000_000_000_000;


I don't get this. If we have BalanceType U64F64 (typecast from u128) this means one cc is 1 << 64, not 1_000_000_000_000_000_000
This needs more explaining comments, even if the new constants already improve readability

@pifragile Why did you arbitrarily chose to set ONE_CC=1_000_000_000_000_000_000? I get that you tried to pick a number that avoids overflow, but your choice has nothing to do with the real interpretation of "one CC". We don't need a power of 10, we need the "real" ONE_CC.

Or: why didn't you just set ONE_CC: u128 = 1 << 64? Then we could typecast this value to U64F64 and would really get 1 CC

@brenzi the rationale behind this is that i assumed that fungibles uses a system similar to Ethereum where numbers are represented as a large integer (like 256 or 128 bit) and a given amount of decimals.
See here: https://github.com/paritytech/substrate/blob/ded44948e2d5a398abcb4e342b0513cb690961bb/frame/support/src/traits/tokens/fungibles/metadata.rs#L29

So for example with 5 decimals, the number x will be represented as x * 10^5. I chose 18 decimals, hence this number.

Ok, but that confirms my concern. We use fixpoint with a base of 2, not base of 10. Unless you can convince me otherwise, I'd like this changed to
ONE_CC: u128 = 1 << 64 throughout

this only works if no client of the balances pallet ever uses the decimals field, because we will obviously not be able to put a value in that field if we use your apporach. i will do some research. what is the reason you would like to change this? to avoid the conversions between base 2 / base 10 numbers?

brenzi

as requested in comments

…e 1<<64 as ONE_CC

This is the node integration of encointer/pallets#190 this only solves the node part. the client doesn't support CC fees yet. (left for #195 to do)

pifragile added 7 commits March 30, 2022 15:09

setup fee payment in community currencies

1da41b4

implement fee conversion factor

7ace4ff

fix f64 nostd issues

ff97003

remove clutter

96d418a

fix float issue

3e6d6e4

move BalanceToCommunityBalance to communities pallet

3e9d4ec

implement test for conversion and fix bugs

0d11d41

pifragile requested review from brenzi and clangenb March 30, 2022 13:12

pifragile mentioned this pull request Mar 30, 2022

Fees in community currency encointer/encointer-node#194

Merged

clangenb reviewed Mar 31, 2022

View reviewed changes

clangenb added 5 commits March 31, 2022 13:12

extract balances-tx-payment-extension crate

66571b8

[balances] remove unused imports

5b126e9

[balances] remove unused variables

09813ba

[balances-tx-payment-extension] tests work

4066d19

[balances-tx-payment-extension] remove unused warnings

9869fd2

clangenb reviewed Mar 31, 2022

View reviewed changes

balances/src/impl_fungibles.rs Show resolved Hide resolved

clangenb added 4 commits March 31, 2022 14:31

[balances-tx-payment-extension] separate aliases from conversion

bfa17db

[balances-tx-payment-extension] improve aliases

1c7b6bd

add license headers

3a97f3b

[balances-tx-payment-ext] use improve type aliases

9e1c30f

clangenb reviewed Mar 31, 2022

View reviewed changes

balances-tx-payment-extension/src/balance_conversion.rs Outdated Show resolved Hide resolved

clangenb added 7 commits March 31, 2022 15:32

drop 'extension' on balances-tx-payment-extension name.

7eb4fcb

[primitives] add EncointerBalanceConverter

f672ef3

[encointer-balances] use balance converter from primitives

767064e

Cargo.lock

a068cda

[balance-tx-payment] incorporate more meaning in the fee_conversion_f…

67c86cb

…actor

move BurnCredit to balances-tx-payement

a4cd038

[balances-tx-payment] simply fee calculation and tests

e92dcc3

clangenb reviewed Apr 1, 2022

View reviewed changes

clangenb added 9 commits April 1, 2022 20:42

[primitives] deterministic fungible to balance type conversion

86ea1c7

[primitives] use rstest

62429fc

[primitives] fix potential overflow in balance to fungible conversion

fc98120

[encointer-balances] move balance_type_to_fungibles tests to primit…

99efdda

…ives

add comment

0996eff

[encointer-balances] better test readability due to alias

15618b5

fmt

68a5dec

fmt

0ee2477

[balance_conversion] use checked math

3074363

clangenb approved these changes Apr 2, 2022

View reviewed changes

[encointer-balances] add comment about InspectMetadata implementation

dd55a87

brenzi reviewed Apr 2, 2022

View reviewed changes

balances-tx-payment/src/balance_conversion.rs Show resolved Hide resolved

clangenb added 3 commits April 2, 2022 09:40

[encointer-balances] adjust unit of FeeConversionFactor to be able …

f5a9350

…to be adjusted more freely

[encointer-balances] adjust unit of FeeConversionFactor to be able …

7ea9b28

…to be adjusted more freely

[balances-tx-payment] rename balance_to_community_balance to `apply_f…

c91ace7

…ee_conversion_factor`

brenzi reviewed Apr 2, 2022

View reviewed changes

brenzi requested changes Apr 3, 2022

View reviewed changes

brenzi and others added 3 commits April 3, 2022 21:46

Merge branch 'master' into fees-in-community-currency

74673ea

rename ONE_CC -> ONE_FUNGIBLE_ASSET and add test for conversion factor

1958adb

added some reasoning on decimals vs base2 and changed unittests to us…

4338fb1

…e 1<<64 as ONE_CC

brenzi approved these changes Apr 4, 2022

View reviewed changes

brenzi merged commit 80442d3 into master Apr 4, 2022

clangenb deleted the fees-in-community-currency branch April 5, 2022 20:49

brenzi pushed a commit to encointer/encointer-node that referenced this pull request Apr 10, 2022

Fees in community currency (#194)

16d459b

This is the node integration of encointer/pallets#190 this only solves the node part. the client doesn't support CC fees yet. (left for #195 to do)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fees in community currency #190

Fees in community currency #190

pifragile commented Mar 30, 2022 •

edited

clangenb Mar 31, 2022

clangenb Mar 31, 2022

pifragile Apr 2, 2022

clangenb Mar 31, 2022

clangenb Mar 31, 2022

clangenb Mar 31, 2022 •

edited

clangenb Mar 31, 2022 •

edited

clangenb Apr 2, 2022

clangenb Apr 1, 2022

clangenb Apr 2, 2022 •

edited

clangenb left a comment •

edited

brenzi Apr 2, 2022 •

edited

brenzi Apr 3, 2022

pifragile Apr 3, 2022

brenzi Apr 3, 2022

pifragile Apr 4, 2022 •

edited

brenzi left a comment

		((fungible_balance << 64) >> 64) as f64 / (10i128.pow(decimals as u32) as f64),
		);

Fees in community currency #190

Fees in community currency #190

Conversation

pifragile commented Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clangenb Mar 31, 2022 • edited

Choose a reason for hiding this comment

clangenb Mar 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clangenb Apr 2, 2022 • edited

Choose a reason for hiding this comment

clangenb left a comment • edited

Choose a reason for hiding this comment

brenzi Apr 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pifragile Apr 4, 2022 • edited

Choose a reason for hiding this comment

brenzi left a comment

Choose a reason for hiding this comment

pifragile commented Mar 30, 2022 •

edited

clangenb Mar 31, 2022 •

edited

clangenb Mar 31, 2022 •

edited

clangenb Apr 2, 2022 •

edited

clangenb left a comment •

edited

brenzi Apr 2, 2022 •

edited

pifragile Apr 4, 2022 •

edited