core

core copied to clipboard

• FIXED:

  • BREAKING: Modify getDetails method in ERC20Standard class to return decimal strings rather than BigNumbers for the fields decimals and balance in the return object. Public controller methods should only ever return JSON typed values. This change brings the ERC20Standard class back into alignment with our API standards.

Code changes look good to me! Except it looks like there is now a drop in coverage.

I was going to suggest ensuring this was covered by unit tests anyway. The fact that the return type of getERC20TokenDecimals was wrong suggests there were gaps in coverage anyway.

Public controller methods should only ever return JSON typed values

I'm not so sure I agree with this. I think if we have a number and we need to represent it as BN, I feel like the controllers repo is the right place to do that. I don't think it is the responsibility of controllers to think about what is JSON-encodable — I think encoding and decoding of BNs (or other values) should be handled in the consumer where it is needed. Thoughts/comments...?

if we have a number and we need to represent it as BN We don't need to represent BN as anything. Any number can be represented by a string.

In the past when we've discussed how to represent numbers on the team, we had agreed to use strings for all interchange rather than bn.js or BigNumber.js because it avoids certain pitfalls and incompatibilities. And more recently we had agreed that it would be better to use BigInteger instead and avoid passing around floats altogether if possible.

I agree that the RPC layer between the extension UI and background isn't a concern for this library, but nonetheless it's probably better to avoid these custom number types in public methods. We only need them for calculations, which can be done entirely within each module/controller.

Coming back to this PR... I've looked through several conversations in Slack where we've talked about big numbers (including the one that spawned this PR) and while I'm not totally clear on all of the pitfalls that the various libraries give us, maybe the fact that all of them go away by using strings is enough. So, I'm good with this. Should we make this more of a guideline: "always represent big numbers as strings until you need to perform calculations on them"?

Pinging here — is this still relevant?

stale