Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Confuse in Balances::transfer logic #722

Closed
atenjin opened this issue Sep 12, 2018 · 1 comment
Closed

Confuse in Balances::transfer logic #722

atenjin opened this issue Sep 12, 2018 · 1 comment
Labels
I3-bug The node fails to follow expected behavior.
Milestone

Comments

@atenjin
Copy link
Contributor

atenjin commented Sep 12, 2018

Hi:
substrate:
commit:3720d74a6fa85d02947ee67b1934c071849b506c
tag:v0.2.15

Question:
When I follow the code in balances module, I'm confused in function transfer() (L278, balances/src/lib.rs):

/// Transfer some liquid free balance to another staker.
	pub fn transfer(origin: T::Origin, dest: Address<T>, value: T::Balance) -> Result {
		let transactor = ensure_signed(origin)?;

		let dest = Self::lookup(dest)?;
		let from_balance = Self::free_balance(&transactor);
		let would_create = from_balance.is_zero();
		let fee = if would_create { Self::creation_fee() } else { Self::transfer_fee() };
		let liability = value + fee;

		let new_from_balance = match from_balance.checked_sub(&liability) {
			Some(b) => b,
			None => return Err("balance too low to send value"),
		};
		if would_create && value < Self::existential_deposit() {
			return Err("value too low to create account");
		}
		T::EnsureAccountLiquid::ensure_account_liquid(&transactor)?;

		let to_balance = Self::free_balance(&dest);
                 // ...
	}

I'm confused for the flag 'would_create'.
For example, if I what to hit the 'would_create' if branch, and then the from_balance must be zero, but the from_balance is belong to origin(the transactor), but if the transactor's balance is zero, It would pass in the next phase:

		let new_from_balance = match from_balance.checked_sub(&liability) {
			Some(b) => b,
			None => return Err("balance too low to send value"),
		};

if we let the value is zero, it could pass, but the next phase:

		if would_create && value < Self::existential_deposit() {
			return Err("value too low to create account");
		}

if the value is zero, the Self::existential_deposit() also must be zero, or else it would return error...

So if the Self::creation_fee(), value, from_balance are all zero,
what's the sense for designing a creation_fee in balances module?
That's what my confusion.

ps:
In the test case, the related params are all zero, so it seems that no one notice this point:
balances/src/mock.rs L53

pub fn new_test_ext(ext_deposit: u64, monied: bool) -> runtime_io::TestExternalities<Blake2Hasher> {
	let mut t = system::GenesisConfig::<Runtime>::default().build_storage().unwrap();
	let balance_factor = if ext_deposit > 0 {
		256
	} else {
		1
	};
	t.extend(GenesisConfig::<Runtime>{
		balances: if monied {
			vec![(1, 10 * balance_factor), (2, 20 * balance_factor), (3, 30 * balance_factor), (4, 40 * balance_factor)]
		} else {
			vec![(10, balance_factor), (20, balance_factor)]
		},
		transaction_base_fee: 0,
		transaction_byte_fee: 0,
		existential_deposit: ext_deposit,
		transfer_fee: 0,
		creation_fee: 0,
		reclaim_rebate: 0,
	}.build_storage().unwrap());
	t.into()
}

In my point, I think the creation_fee is using for the cast for creating a new account, like what "eos" do.
but there is something wrong in would_create.
Does the would_create flag should from the dest.balance? not the origin.balance
like:

	pub fn transfer(origin: T::Origin, dest: Address<T>, value: T::Balance) -> Result {
		let transactor = ensure_signed(origin)?;

		let dest = Self::lookup(dest)?;
		let from_balance = Self::free_balance(&transactor);
                let to_balance = Self::free_balance(&dest);  // that's where would_create from
		let would_create = to_balance.is_zero();
@gavofyork gavofyork added the I3-bug The node fails to follow expected behavior. label Sep 12, 2018
@gavofyork
Copy link
Member

indeed looks like a bug - feel free to submit a PR! :)

@gavofyork gavofyork added this to the 1.0 beta milestone Sep 12, 2018
atenjin pushed a commit to atenjin/substrate that referenced this issue Sep 13, 2018
issue:paritytech#722
would_create flag should depend on dest, not origin.
change 
```rust
let would_create = from_balance.is_zero();
```
to 
```rust
    let to_balance = Self::free_balance(&dest); 
    let would_create = to_balance.is_zero(); 
```
in the other hand, provide `fn new_test_ext2()` and let `transfer_fee=10`, `creation_fee=50` for test case
@atenjin atenjin closed this as completed Sep 13, 2018
gavofyork pushed a commit that referenced this issue Sep 13, 2018
* bugfix: balances::transfer for new_account

issue:#722
would_create flag should depend on dest, not origin.
change 
```rust
let would_create = from_balance.is_zero();
```
to 
```rust
    let to_balance = Self::free_balance(&dest); 
    let would_create = to_balance.is_zero(); 
```
in the other hand, provide `fn new_test_ext2()` and let `transfer_fee=10`, `creation_fee=50` for test case

* Update lib.rs

* Update tests.rs

* Make `impl_outer_origin!` support generic `Origin`s (#732)

* Make `impl_outer_origin!` support generic `Origin`s

* Support empty outer origin

* Contracts: fix transfer function. (#733)

* Remove dependency on the parity repo (#734)

* Fix test

* Anothe fix
helin6 pushed a commit to boolnetwork/substrate that referenced this issue Jul 25, 2023
* fix(ci): wasm tests with cache

* disable logging in wasm tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I3-bug The node fails to follow expected behavior.
Projects
None yet
Development

No branches or pull requests

2 participants