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

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Apr 18, 2019

This PR is nowhere near being mergable. It is just an first attempt to remove as<u64> from simple arithmetic and show progress on it.

For now it only add a generic BlockNumber to some structures in core.

Current issue:

  • to build proof client make use of encode_cht_key which convert blocknumber to u64, I don't know that much about that part and how it should be changed yet.

@gui1117 gui1117 added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Apr 18, 2019
@kianenigma
Copy link
Contributor

Is this only an attempt to clean the core and block number from As<u64> or everything related to it?

I remember this approach suggested in a comment and it seems like it should be applied to all modules, including those that use block number etc:

  • We remove As<u64>.
  • If a module needs to convert u64 -> XXX We implement a new TryFrom<u64> for XXX, XXX being Balance, BlockNumber or other primitive substrate types that are currently entangled with As<u64>.
  • There will be no backward default implementation. If a module wants to do the reverse, XXX -> u64, it must pass it in as a config type implementing Convert<XXX, u64>. The Staking module currently has an ugly version of this (with the issue that it implements converts on both sides. I actually placed a note to remove it as soon as TryFrom is implemented).

@bkchr bkchr added A0-please_review Pull request needs code review. and removed A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). labels Apr 19, 2019
@gui1117
Copy link
Contributor Author

gui1117 commented Apr 21, 2019

We can do that step by step. First I wanted to remove As from simple arithmetic and see what is the codebase that make use of it. This PR was just a draft to see what was affected.

It appears to be wideley used by blocknumber to be converted back and forth. limiting its actual genericity.

I think the conclusion from discussions was to make blocknumber generic everywhere. (which is borring because we add a new parameter to every structure there, or maybe we can make a trait with associated type but then deriving is wrong so has to be done manually).

About modules usage of As I think they should be removed as you suggested.

Hmm I pushed the PR to show my research but this is not a PR. Maybe I should clause it and just linked the branch through as issue..

@bkchr
Copy link
Member

bkchr commented Apr 23, 2019

Okay, then I close this pr.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants