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

Refactor CurrencyToVote#6896

Merged
13 commits merged intomasterfrom
kiz-refactor-currency-to-vote
Oct 8, 2020
Merged

Refactor CurrencyToVote#6896
13 commits merged intomasterfrom
kiz-refactor-currency-to-vote

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 a review from andresilva as a code owner August 14, 2020 15:36
@kianenigma kianenigma requested review from shawntabrizi and tomusdrw and removed request for andresilva August 17, 2020 09:14
kianenigma
kianenigma commented Aug 17, 2020
View reviewed changes
bkchr
bkchr reviewed Aug 17, 2020
View reviewed changes
tomusdrw
tomusdrw reviewed Aug 17, 2020
View reviewed changes
frame/support/src/traits.rs Outdated

/// 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

@tomusdrw tomusdrw Aug 17, 2020

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

@kianenigma kianenigma Aug 17, 2020

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.

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

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

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 reviewed Aug 18, 2020
View reviewed changes
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

kianenigma commented Aug 21, 2020

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

kianenigma commented Sep 10, 2020

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

kianenigma commented Oct 2, 2020

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

kianenigma commented Oct 7, 2020

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

@kianenigma
Copy link
Contributor Author

kianenigma commented Oct 8, 2020

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

4 more 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

Reviewers whose approvals may not affect merge requirements

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