-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
Set Existential Deposit to Zero and Refactor Related Tests #170
Conversation
/describe |
/review |
…seio/laos into feature/existential_deposit_is_0
/describe |
/review |
/describe |
/describe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is e2e test missing as per acceptance criteria:
I see an account existing with 1e-18 DROP
I can transfer 1e-18 DROP to an account that had no balance, and check it has 1Wei
@@ -181,7 +181,7 @@ fn testnet_genesis( | |||
}, | |||
collator_selection: laos_ownership_runtime::CollatorSelectionConfig { | |||
invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(), | |||
candidacy_bond: EXISTENTIAL_DEPOSIT * 16, | |||
candidacy_bond: laos_ownership_runtime::ExistentialDeposit::get() * 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are requiring 0 bond for candidacy to be a collator? This should be a non-zero value, IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please open an issue on the topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, if you are changing something in the code, then everything that depends on this constant should be addressed as well. I suggest at least creating a new constant that does not depend on the ED
anymore. Value can be calculated with previous non-zero value of ED
16 * PREVIOUS_ED
ownership-chain/runtime/src/tests.rs
Outdated
let result = Balances::deposit(&alice, minimum_amount, Precision::Exact); | ||
|
||
// I am forced to use match cause result doesn't implement Debug trait | ||
match result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_ok!()
didn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it doesn't because it needs Debug trait to be implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert!(Balances::deposit(&alice, minimum_amount, Precision::Exact).is_ok());
ownership-chain/runtime/src/tests.rs
Outdated
let result = Balances::deposit(&alice, minimum_amount, Precision::Exact); | ||
|
||
// I am forced to use match cause result doesn't implement Debug trait | ||
match result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert!(Balances::deposit(&alice, minimum_amount, Precision::Exact).is_ok());
@@ -181,7 +181,7 @@ fn testnet_genesis( | |||
}, | |||
collator_selection: laos_ownership_runtime::CollatorSelectionConfig { | |||
invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(), | |||
candidacy_bond: EXISTENTIAL_DEPOSIT * 16, | |||
candidacy_bond: laos_ownership_runtime::ExistentialDeposit::get() * 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, if you are changing something in the code, then everything that depends on this constant should be addressed as well. I suggest at least creating a new constant that does not depend on the ED
anymore. Value can be calculated with previous non-zero value of ED
16 * PREVIOUS_ED
I didn't see that the value of candidacy_bond was not protected from a possible existential deposit of 0 .. I fixed it to avoid it to be 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
only 2 really small nitpicks
@@ -181,7 +181,7 @@ fn testnet_genesis( | |||
}, | |||
collator_selection: laos_ownership_runtime::CollatorSelectionConfig { | |||
invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(), | |||
candidacy_bond: EXISTENTIAL_DEPOSIT * 16, | |||
candidacy_bond: laos_ownership_runtime::ExistentialDeposit::get() * 16 + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first part of multiplication resolves to 0 anyway
candidacy_bond: laos_ownership_runtime::ExistentialDeposit::get() * 16 + 1, | |
candidacy_bond: laos_ownership_runtime::WEI, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues with Using laos_ownership_runtime::WEI
for candidacy_bond
-
Contextual Inappropriateness:
- WEI, as a denomination, is specific to Ethereum.
-
Potential Inconsistency with
ExistentialDeposit
:- Setting the
candidacy_bond
to a constant value like WEI could create a situation where it is less than theExistentialDeposit
. Such a discrepancy could lead to undesired behaviors.
- Setting the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Contextual Inappropriateness:
I agree that WEI is not completely appropriate, but I suggested it for the sake of having some relation to constants defined in the runtime. Ideally, there should be a visible pub const COLLATOR_CANDIDACY_BOND: u128
in the runtime or the value should be at least tied to a constant in the runtime. In any case, using WEI is still better than having simply 1, imo.
- Potential Inconsistency with ExistentialDeposit:
the point of the PR is to make ED
zero, why would there be a situation where candidacy_bond
is less than zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We concur that using WEI is inappropriate due to its lack of direct relevance in a Substrate context.
- Regarding the use of a constant, it's crucial to maintain the dependency of
candidacy_bond
onExistentialDeposit
. Even ifExistentialDeposit
is set to zero in current configuration, using a fixed constant forcandidacy_bond
introduces a risk. This approach could lead to a problematic situation in scenarios where the runtime is configured with anExistentialDeposit
greater than zero. In such cases, the candidacy_bond might inadvertently fall below theExistentialDeposit
, leading to unintended consequences. Therefore, tyingcandidacy_bond
to a dynamic value likeExistentialDeposit
is more robust and adaptable to different runtime configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's crucial to maintain the dependency of candidacy_bond on ExistentialDeposit. Even if ExistentialDeposit is set to zero in current configuration, using a fixed constant for candidacy_bond introduces a risk.
why is it "crucial" to maintain dependency and why there should be a dependency? ED was simply used because there was a need to have some unambiguous value. There is no special relationship between candidacy_bond
and ED
. In the same way, you can use UNIT
or any other constant from runtime to derive the value of candidacy_bond
.
in any case, I looked at other parachains, some set it to a big value, like astar:
some others set it to 0:
So, I suggest we either have it explicitly as Zero::zero()
or have some non-zero value defined clearly
/describe |
/describe |
ownership-chain/runtime/src/tests.rs
Outdated
} | ||
|
||
#[test] | ||
fn deposit_smallest_unit_should_succeed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with send_1_wei_to_wallet_with_0_balance_should_increase_balance_by_1_wei
, this test becomes unnecessary, since all the actions/checks that are done here is done in send_1_wei_to_wallet_with_0_balance_should_increase_balance_by_1_wei
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
@@ -181,7 +181,7 @@ fn testnet_genesis( | |||
}, | |||
collator_selection: laos_ownership_runtime::CollatorSelectionConfig { | |||
invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(), | |||
candidacy_bond: EXISTENTIAL_DEPOSIT * 16, | |||
candidacy_bond: laos_ownership_runtime::ExistentialDeposit::get() + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my initial comment about this was wrong, this can actually be Zero::zero()
and doesn't need to be a non-zero value. Assuming that, can we set this to either:
- bigger explicit value, sth like
UNIT * 16
- or to
Zero::zero()
explicitly
I am just suggesting it since it seems this is how other parachains are doing it. At the very least, I am ok with having it simply 1, tying it to ED
doesn't really make much sense to me though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm: the zero option is nice and already tested in other blockchain
/describe |
PR Type:
Refactoring
PR Description:
This PR primarily focuses on setting the Existential Deposit to zero and refactoring the related tests. The main changes include:
PR Main Files Walkthrough:
files:
ownership-chain/node/src/chain_spec.rs
: The candidacy bond in the collator selection configuration is set to zero.ownership-chain/runtime/src/lib.rs
: The Existential Deposit is set to zero, with an explanation of the reasons for this change.ownership-chain/runtime/src/tests.rs
: Tests have been added and refactored to reflect the changes in the Existential Deposit and candidacy bond.ownership-chain/runtime/Cargo.toml
: The 'pallet-balances' feature 'insecure_zero_ed' has been enabled.