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.

Sign up for GitHub

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

Jump to bottom

Fees in community currency #190

Merged
merged 39 commits into from
Apr 4, 2022
Merged

Fees in community currency #190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
1da41b4
setup fee payment in community currencies
pifragile Mar 22, 2022
7ace4ff
implement fee conversion factor
pifragile Mar 30, 2022
ff97003
fix f64 nostd issues
pifragile Mar 30, 2022
96d418a
remove clutter
pifragile Mar 30, 2022
3e6d6e4
fix float issue
pifragile Mar 30, 2022
3e9d4ec
move BalanceToCommunityBalance to communities pallet
pifragile Mar 30, 2022
0d11d41
implement test for conversion and fix bugs
pifragile Mar 30, 2022
66571b8
extract balances-tx-payment-extension crate
clangenb Mar 31, 2022
5b126e9
[balances] remove unused imports
clangenb Mar 31, 2022
09813ba
[balances] remove unused variables
clangenb Mar 31, 2022
4066d19
[balances-tx-payment-extension] tests work
clangenb Mar 31, 2022
9869fd2
[balances-tx-payment-extension] remove unused warnings
clangenb Mar 31, 2022
bfa17db
[balances-tx-payment-extension] separate aliases from conversion
clangenb Mar 31, 2022
1c7b6bd
[balances-tx-payment-extension] improve aliases
clangenb Mar 31, 2022
3a97f3b
add license headers
clangenb Mar 31, 2022
9e1c30f
[balances-tx-payment-ext] use improve type aliases
clangenb Mar 31, 2022
7eb4fcb
drop 'extension' on `balances-tx-payment-extension` name.
clangenb Mar 31, 2022
f672ef3
[primitives] add `EncointerBalanceConverter`
clangenb Mar 31, 2022
767064e
[encointer-balances] use balance converter from primitives
clangenb Mar 31, 2022
a068cda
Cargo.lock
clangenb Mar 31, 2022
67c86cb
[balance-tx-payment] incorporate more meaning in the fee_conversion_f…
clangenb Apr 1, 2022
a4cd038
move `BurnCredit` to `balances-tx-payement`
clangenb Apr 1, 2022
e92dcc3
[balances-tx-payment] simply fee calculation and tests
clangenb Apr 1, 2022
86ea1c7
[primitives] deterministic fungible to balance type conversion
clangenb Apr 1, 2022
62429fc
[primitives] use rstest
clangenb Apr 1, 2022
fc98120
[primitives] fix potential overflow in balance to fungible conversion
clangenb Apr 2, 2022
99efdda
[encointer-balances] move `balance_type_to_fungibles` tests to primit…
clangenb Apr 2, 2022
0996eff
add comment
clangenb Apr 2, 2022
15618b5
[encointer-balances] better test readability due to alias
clangenb Apr 2, 2022
68a5dec
fmt
clangenb Apr 2, 2022
0ee2477
fmt
clangenb Apr 2, 2022
3074363
[balance_conversion] use checked math
clangenb Apr 2, 2022
dd55a87
[encointer-balances] add comment about `InspectMetadata` implementation
clangenb Apr 2, 2022
f5a9350
[encointer-balances] adjust unit of `FeeConversionFactor` to be able …
clangenb Apr 2, 2022
7ea9b28
[encointer-balances] adjust unit of `FeeConversionFactor` to be able …
clangenb Apr 2, 2022
c91ace7
[balances-tx-payment] rename balance_to_community_balance to `apply_f…
clangenb Apr 2, 2022
74673ea
Merge branch 'master' into fees-in-community-currency
brenzi Apr 3, 2022
1958adb
rename ONE_CC -> ONE_FUNGIBLE_ASSET and add test for conversion factor
pifragile Apr 4, 2022
4338fb1
added some reasoning on decimals vs base2 and changed unittests to us…
brenzi Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 17 additions & 5 deletions balances-tx-payment/src/balance_conversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,32 @@ use pallet_encointer_balances::Pallet as BalancesPallet;
use pallet_encointer_communities::{Config as CommunitiesConfig, Pallet as CommunitiesPallet};
use sp_runtime::traits::Convert;

/// 1 micro KSM with 12 decimals
pub const ONE_MICRO_KSM: u128 = 1_000_000;

/// 1 KSM with 12 decimals
pub const ONE_KSM: u128 = 1_000_000 * ONE_MICRO_KSM;

/// 1 Kilo-KSM with 12 decimals
pub const ONE_KILO_KSM: u128 = 1_000 * ONE_KSM;

/// Transforms the native token to the community currency
///
/// Assumptions:
/// * Native token has 12 decimals
/// * fee_conversion_factor is in Units 1 / [KSM]
/// * fee_conversion_factor is in Units 1 / [pKSM]
///
/// Applies the formula:
///
pub fn balance_to_community_balance(
balance: u128, // Unit = [pKSM]
balance: u128,
reward: u128,
fee_conversion_factor: u32,
fee_conversion_factor: u128,
) -> u128 {
brenzi marked this conversation as resolved.
Show resolved Hide resolved
return balance
.saturating_mul(fee_conversion_factor as u128)
.saturating_mul(reward)
.checked_div(1_000_000_000_000) // <- unit discrepancy: balance [pKSM] vs. fee_conversion_factor [KSM]
.saturating_mul(fee_conversion_factor as u128)
.checked_div(ONE_KILO_KSM) // <- unit discrepancy: balance [pKSM] vs. fee_conversion_factor [KKSM]
.expect("Divisor != 0; qed")
}

Expand Down
15 changes: 7 additions & 8 deletions balances-tx-payment/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
use crate::balance_to_community_balance;
use crate::{balance_to_community_balance, ONE_MICRO_KSM};
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;
Copy link
Member

@brenzi brenzi Apr 2, 2022 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pifragile pifragile Apr 4, 2022 •
edited

Choose a reason for hiding this comment

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

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?


#[rstest(ksm_balance, ceremony_reward, conversion_factor, expected_community_balance,
case(5 * ONE_MICRO_KSM, 20 * ONE_CC, 100, ONE_CC / 100),
case(10 * ONE_MICRO_KSM, 20 * ONE_CC, 100, ONE_CC / 50),
case(5 * ONE_MICRO_KSM, 10 * ONE_CC, 100, ONE_CC / 200),
case(5 * ONE_MICRO_KSM, 20 * ONE_CC, 100_000, ONE_CC / 100),
case(10 * ONE_MICRO_KSM, 20 * ONE_CC, 100_000, ONE_CC / 50),
case(5 * ONE_MICRO_KSM, 10 * ONE_CC, 100_000, ONE_CC / 200),
case(5_000 * ONE_MICRO_KSM, 20 * ONE_CC, 100_000, ONE_CC * 10),
case(5 * ONE_MICRO_KSM, 20_000_000 * ONE_CC, 100_000, ONE_CC * 10_000),
)]
fn balance_to_community_balance_works(
ksm_balance: u128,
ceremony_reward: u128,
conversion_factor: u32,
conversion_factor: u128,
expected_community_balance: u128,
) {
let balance = balance_to_community_balance(ksm_balance, ceremony_reward, conversion_factor);
Expand Down
4 changes: 2 additions & 2 deletions balances/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,15 @@ pub mod pallet {
///
/// [CC]: 1 Unit of Community Currency
/// NI: Nominal Income. Unit = [CC]
/// FCF: Fee Conversion Factor. Unit = [1/KSM]
/// FCF: Fee Conversion Factor. Unit = [1/ KKSM] <- Kilo-KSM to be able to adjust fee factor in both ways.
/// CB: Balance in Community Currency [CC]
///
/// The following equation should hold for fee design:
/// KSM * FCF * NI = CB -> FCF = CB / (NI * KSM)
///
/// Example to get 0.01 [CC] for 5 [muKSM] and a NI of 20 [CC].
///
/// FCF = 0.01 / (20 * 5*10^-6 [KSM]) = 0.01 / (20 * 5 * 10e-6) = 100
/// FCF = 0.01 / (20 * 5*10^-12 [KKSM]) = 0.01 / (20 * 5 * 10e-12) = 100_000
pub fee_conversion_factor: FeeConversionFactorType,
}

Expand Down
10 changes: 5 additions & 5 deletions balances/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ fn set_fee_conversion_factor_errs_with_bad_origin() {
assert_dispatch_err(
EncointerBalances::set_fee_conversion_factor(
Origin::signed(AccountKeyring::Bob.into()),
5u32,
5,
),
DispatchError::BadOrigin,
);
Expand All @@ -180,12 +180,12 @@ fn set_fee_conversion_factor_errs_with_bad_origin() {
#[test]
fn set_fee_conversion_factor_works() {
new_test_ext().execute_with(|| {
assert_ok!(EncointerBalances::set_fee_conversion_factor(Origin::signed(master()), 5u32));
assert_ok!(EncointerBalances::set_fee_conversion_factor(Origin::signed(master()), 5));

assert_eq!(EncointerBalances::fee_conversion_factor(), 5u32);
assert_ok!(EncointerBalances::set_fee_conversion_factor(Origin::signed(master()), 6u32,));
assert_eq!(EncointerBalances::fee_conversion_factor(), 5);
assert_ok!(EncointerBalances::set_fee_conversion_factor(Origin::signed(master()), 6));

assert_eq!(EncointerBalances::fee_conversion_factor(), 6u32);
assert_eq!(EncointerBalances::fee_conversion_factor(), 6);
});
}

Expand Down
2 changes: 1 addition & 1 deletion primitives/src/balances.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub type BalanceType = I64F64;
/// 1.3188e-07 : halving time of 1 year if blocktime is 6s
pub type Demurrage = I64F64;

pub type FeeConversionFactorType = u32;
pub type FeeConversionFactorType = u128;

#[derive(
Encode, Decode, Default, RuntimeDebug, Clone, Copy, PartialEq, TypeInfo, MaxEncodedLen,
Expand Down