polkadot-dropit / node

Node for the DropIt Chain
https://polkadot-dropit.github.io/
The Unlicense
0 stars 0 forks source link

pallet-balances configuration might need adjustment because ED is 0 #12

Open gui1117 opened 5 months ago

gui1117 commented 5 months ago

there is those issues: https://github.com/paritytech/substrate/issues/10117 https://github.com/paritytech/polkadot-sdk/issues/337

I proposed a fix on the configurations of the pallet: https://github.com/paritytech/substrate/issues/10117#issuecomment-955718519

that said I don't remember any details

Also some warning in the code:

        /// The minimum amount required to keep an account open. MUST BE GREATER THAN ZERO!
        ///
        /// If you *really* need it to be zero, you can enable the feature `insecure_zero_ed` for
        /// this pallet. However, you do so at your own risk: this will open up a major DoS vector.
        /// In case you have multiple sources of provider references, you may also get unexpected
        /// behaviour if you set this to zero.
        ///
        /// Bottom line: Do yourself a favour and make it at least one!
        #[pallet::constant]
        #[pallet::no_default_bounds]
        type ExistentialDeposit: Get<Self::Balance>;
gui1117 commented 5 months ago

if ED is 0 then deposit_into_existing and repatriate_reserved are broken.

Eiter we try to put type AccountData = Option<super::AccountData<u64>>; on the configuration of frame_system or we put ED to be 1 unit.

to convince yourself add those test in polkadot-sdk/substrate/pallet/balances/src/tests/currency_test.rs and run cargo test -p pallet-balances --features insecure_zero_ed

#[test]
fn deposit_into_existing_for_existing_zero_account() {
    // These functions all use `mutate_account` which may introduce a storage change when
    // the account never existed to begin with, and shouldn't exist in the end.
    ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| {
        let alice = 666;
        let bob = 777;
        assert_ok!(Balances::force_set_balance(frame_system::Origin::<Test>::Root.into(), alice, 2000));
        assert_ok!(Balances::force_transfer(frame_system::Origin::<Test>::Root.into(), alice, bob, 2000));

        dbg!(666, frame_system::Account::<Test>::get(alice));
        dbg!(777, frame_system::Account::<Test>::get(bob));

        let _ = dbg!(Balances::deposit_into_existing(
                &alice,
                500,
        )).unwrap();
    });
}

#[test]
fn repatriate_reserved_works_for_zero_account() {
    // These functions all use `mutate_account` which may introduce a storage change when
    // the account never existed to begin with, and shouldn't exist in the end.
    ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| {
        let alice = 666;
        let bob = 777;
        assert_ok!(Balances::force_set_balance(frame_system::Origin::<Test>::Root.into(), alice, 2000));
        assert_ok!(Balances::force_transfer(frame_system::Origin::<Test>::Root.into(), alice, bob, 2000));

        dbg!(666, frame_system::Account::<Test>::get(alice));
        dbg!(777, frame_system::Account::<Test>::get(bob));

        assert_ok!(Balances::reserve(&bob, 500));

        dbg!(alice, frame_system::Account::<Test>::get(alice));
        dbg!(bob, frame_system::Account::<Test>::get(bob));

        // Repatriate Reserve
        dbg!(Balances::repatriate_reserved(
                &bob,
                &alice,
                500,
                crate::Status::Free
        )).unwrap();
    });
}
ggwpez commented 5 months ago

I thought it could be done easier like https://github.com/paritytech/polkadot-sdk/pull/3903, but does not seem like it.
So the type AccountData = Option<super::AccountData<u64>>; is probably the easiest way for now.

shawntabrizi commented 5 months ago

For the purposes of DropIt, we def want ED to be 0. Any non-zero value won't work for effortless distribution.

gui1117 commented 5 months ago

what is the problem with having ED to be 1 unit? like 0.0000....0001 DOT

The dust when going below ED would always be 0.

I'm afraid some issue will arise when deposit_into_existing or repatriate_reserved get used indirectly.

It is like a sword of Damocles