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

Treat block numbers as strings, not BigNums. #5449

Merged
merged 1 commit into from Apr 27, 2017
Merged

Conversation

gavofyork
Copy link
Contributor

No description provided.

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Apr 12, 2017
@gavofyork gavofyork requested a review from ngotchac April 12, 2017 11:57
@gavofyork gavofyork changed the title Treat clock numbers as strings, not BigNums. Treat block numbers as strings, not BigNums. Apr 12, 2017
@ngotchac
Copy link
Contributor

I think that this should fix the bug : https://github.com/paritytech/parity/pull/5405/files#diff-82dc1975e34f0323a02d1c76fe49530cR69 We don't really use implicit casting to Numbers anywhere else in the code, might want to keep it that way

@gavofyork
Copy link
Contributor Author

that won't fix transactionReceipt.blockNumber being a string though, will it?

@ngotchac
Copy link
Contributor

That's right, but it shouldn't be a string AFAICT. Is it?

@gavofyork
Copy link
Contributor Author

it failed in the same way - might have been a bignum.

@jacogr
Copy link
Contributor

jacogr commented Apr 19, 2017

We don't actually format blockHeight as BigNumber in the output formatter, so it just passes what it receives from the RPC call. For consistency, it should be done since all numbers are passed as BigNumbers.

(blockNumber is a BigNumer, so it should not have had the same issue)

@ngotchac
Copy link
Contributor

But blockHeight is not returned by the RPC call, it's computed on the JS side.

@gavofyork
Copy link
Contributor Author

bignumber still breaks. it is expecting a straight JS number. all of those type coersions are needed for it not to break for me.

@jacogr
Copy link
Contributor

jacogr commented Apr 26, 2017

Well, the approach works. However, now that we display via +blockHeight/blockNumber we are not consistent with what we have in the UI, i.e. all blockNumbers are

  1. treated as BigNumbers (calculated and passed around)
  2. formatted using toFixed(), i.e. will display 3,456,789 instead of 3456789 as it does in this case

The preferred approach would have been to calculate the blockHeight as a BigNumber and use that, instead of making it a string

Making as mustnt-grumble since it solves the immediate issue, however should be fixed at the route to be consistent in a follow-up.

@jacogr jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 26, 2017
@gavofyork gavofyork merged commit 43175f1 into master Apr 27, 2017
@gavofyork gavofyork deleted the fix-mined-dialog branch April 27, 2017 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants