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

Fix Dev chain balance values. #2302

Closed
wants to merge 8 commits into from
Closed

Fix Dev chain balance values. #2302

wants to merge 8 commits into from

Conversation

kianenigma
Copy link
Contributor

all explained in #2301.

Will probably either get merged as is, as a quick temp fix (after removing prints) or will be changed to in-progress for a while.

@kianenigma kianenigma added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Apr 16, 2019
@kianenigma kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A4-gotissues labels Apr 17, 2019
@@ -221,6 +221,7 @@ mod tests {
}

#[test]
#[ignore]
Copy link
Member

Choose a reason for hiding this comment

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

Please just merge master^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged.

// parameter in the system. It should be declared, the assertion should change accordingly and it should
// influence the value of shift in Staking's `CurrencyToVote`.
assert!(
balance > BalanceOf::<T>::sa(min_staked_balance),
Copy link
Member

Choose a reason for hiding this comment

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

>=, no?

@gavofyork
Copy link
Member

i guess this is ok for now, but i'd really like to implement multiplication/overflow properly rather than relying on mismatched sizes of votes & balances.

@kianenigma
Copy link
Contributor Author

i guess this is ok for now, but i'd really like to implement multiplication/overflow properly rather than relying on mismatched sizes of votes & balances.

agreed but even if we had floating point numbers available (= no worrying about arithmetic), this issue would still be an option. They are related but not 100% the same thing.

The emphasis here is that if balances less than N bit are deemed to be dust, it should be indicated somewhere as a parameter because a module might depend on it. This is exactly the relationship between Staking and how polkadot implements balance in a way that 32 bits is dust.

I actually don't have a strong inclination to merge this as-is and prefer discussing it more first (was expecting a bit more feedback on the issue honestly). Maybe this new parameter should be stated somewhere else (e.g. in balances)?

But if we want a cleaner dev chain asap this can be merged and will not have much of an impact except one new parameter to Staking's genesis. The entire feature is also effectively disabled if the value is 0.

@gavofyork
Copy link
Member

gavofyork commented Apr 23, 2019

If we didn't need to worry about precision then we'd not bother shifting at all and this would all become moot.

The correct way to solve this isn't through a spurious notion of "dust" with insignificant lower-bits and unused upper-bits. The correct way is to equate votes to balance and use overflow-safe arithmetic.

@gavofyork gavofyork closed this Apr 24, 2019
@bkchr bkchr deleted the kiz-dev-build branch April 24, 2019 09:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants