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

Refactor CurrencyToVote #6896

Merged
13 commits merged into from
Oct 8, 2020
Merged

Refactor CurrencyToVote #6896

13 commits merged into from
Oct 8, 2020

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Aug 14, 2020 •
edited
Loading

Both staking and elections-phragmen need Type CurrencyToVote that helps them convert T::Currency::Balance into u64. This values is then later on used for complex arithmetic where overflow might happen.

This PR refactors this in a few ways:

  1. Move away from Convert to a custom trait that accepts total_issuance as parameter. This avoid all the numerous calls to T::Currency::TotalIssuance(). Indeed, these calls will be a hit in the overlay cache but still it will incur some overhead, given that we were calling it many many many times (i.e. once per nominator, multiple times -- this is the order).
  2. Move everything, with much better documentation to support to allow reuse.

polkadot companion: paritytech/polkadot#1610

@kianenigma kianenigma added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Aug 14, 2020
@kianenigma kianenigma requested review from tomusdrw and shawntabrizi and removed request for andresilva August 17, 2020 09:14
kianenigma
kianenigma commented Aug 17, 2020
View reviewed changes
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
tomusdrw
tomusdrw reviewed Aug 17, 2020
View reviewed changes
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Show resolved Hide resolved
frame/support/src/traits.rs Outdated
@@ -1664,6 +1665,70 @@ pub trait Instance: 'static {
const PREFIX: &'static str ;
}

/// A trait similar to `Convert` to convert values from `B` (i.e. the `Balance` type of the chain)
/// into u64, which is the standard numeric types used in election and other places where complex
/// calculation over balance type is needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the trait docs hint what to do with overflows? It seems that this might be a lossy conversion and it's not clear to me how to implement such trait for say U256 balance type just by looking at the trait definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think end of the day that depends on the chain's logic and their builders decision.

In other words, I cannot give them a definitive advise here of what to do :D so I will just leave it open.

frame/support/src/traits.rs Outdated Show resolved Hide resolved
gui1117
gui1117 reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

implementation looks good to me

frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/support/src/traits.rs Outdated Show resolved Hide resolved
kianenigma and others added 3 commits August 17, 2020 16:20
@kianenigma @gui1117
Update frame/support/src/traits.rs
a68c4a1 
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@kianenigma
Some grumbles
78993cb
@kianenigma
Merge branch 'kiz-refactor-currency-to-vote' of github.com:paritytech…
13defa4 
…/substrate into kiz-refactor-currency-to-vote
gui1117
gui1117 approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

lgtm

frame/support/src/traits.rs Outdated Show resolved Hide resolved
tomusdrw
tomusdrw approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm % Gui's grumbles

shawntabrizi
shawntabrizi approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

afaict looks good, although if anyone uses this new currency to vote in storage, bad things may happen. user discretion advised.

@kianenigma kianenigma mentioned this pull request Aug 20, 2020
Merged
@kianenigma kianenigma added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Aug 20, 2020
@kianenigma kianenigma added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed A8-mergeoncegreen labels Aug 21, 2020
@kianenigma
Copy link
Contributor Author

I am being a bit conservative here with the audit request. The PR itself really only duplicates the previous code, in a nicer and more optimised way. Nonetheless, I think the old logic of currency -> vote which has been silently propagated everywhere itself is worthy of an audit.

@kianenigma
Copy link
Contributor Author

Keeping up to date; still needs an audit.

@bkchr
Copy link
Member

bkchr commented Oct 1, 2020

Also needs a master merge :D

@kianenigma
Copy link
Contributor Author

Should be first audited anyhow. Hopefully next week

@viniul
Copy link
Contributor

viniul commented Oct 7, 2020

We are done with our review for this PR and don't see any issues with it, lgtm!

@kianenigma kianenigma added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed A1-onice D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Oct 7, 2020
@kianenigma
Copy link
Contributor Author

I want to do another self review tomorrow and then we proceed with the merge.

@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 8, 2020

Trying merge.

@ghost ghost merged commit 90e8abf into master Oct 8, 2020
@ghost ghost deleted the kiz-refactor-currency-to-vote branch October 8, 2020 14:50
ordian added a commit that referenced this pull request Oct 9, 2020
@ordian
Merge branch 'master' into ao-network-expose-incremental-priority-gro…
f089aaf 
…up-updates

* master:
  Async keystore + Authority-Discovery async/await (#7000)
  Fixes logging of target names with dashes (#7281)
  seal: Add automated weights for contract API calls (#7017)
  add ss58 id for nodle (#7279)
  Refactor CurrencyToVote (#6896)
  bump-allocator: document & poison (#7277)
  Reset flaming fir network (#7274)
  reschedule (#6860)
  Drop system cache for trie benchmarks (#7242)
  client: improve log formatting (#7272)
  Rework `InspectState` (#7271)
  SystemOrigin trait (#7226)
  Update ss58 registry for Dock network (#7263)
  .maintain/monitoring: Add alert when continuous task ends (#7250)
  Rename `TRANSACTION_VERSION` to `EXTRINSIC_VERSION` (#7258)
  Split block announce processing into two parts (#6958)
  Fix offchain election to respect the weight (#7215)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr left review comments

@gui1117 gui1117 gui1117 approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kianenigma @bkchr @viniul @tomusdrw @shawntabrizi @gui1117 @gnunicorn @paritytech-ci