feat: add a decimals parameter, and decimals method to brand #1964
Conversation
There was a problem hiding this comment.
The downside is that this hardcodes a certain interpretation.
I think the right way to think of this is that 'decimals' tells the display software which position corresponds to whole numbers. If the display wants to display Millions, then it knows that it should be millions of that unit, rather than millions of the most precise division.
if mills are the unit, and decimals is set to 3, then
- 3000 is $3.00
- 3 000 000 is $3000.00
- 3 000 000 000 could be rendered $3.0M
katelynsills
force-pushed
the
119-decimal
branch
from
8b57c4b to
153caff
Compare
|
Addressed PR comments. In order to ensure |
katelynsills
force-pushed
the
119-decimal
branch
from
28ee0c8 to
104a1a0
Compare
packages/ERTP/src/types.js
Outdated
| * numbers. We require fungible digital assets to be represented in | ||
| * integers, in the smallest unit (i.e. USD might be represented in mill, | ||
| * a thousandth of a dollar. In that case, `decimalPlaces` would be 3.) | ||
| * For non-fungible digital assets, this should be left as undefined. |
There was a problem hiding this comment.
DisplayInfo should be expected to grow over time as one would expect of an options bag. Each use would only contain properties appropriate for the kind of amount math it is used on. Rather than having decimalPlaces be undefined, the property should be optional. AFAICT your assertDisplayInfo already allows it to be absent so it is just an issue of how you type and describe it here.
There was a problem hiding this comment.
ah, yep, that is what I intended. Will change
| @@ -23,7 +23,7 @@ function makeIssuerKit( | |||
| displayInfo = undefined, | |||
| ) { | |||
| assert.typeof(allegedName, 'string'); | |||
| assertDisplayInfo(displayInfo); | |||
| displayInfo = coerceDisplayInfo(displayInfo); | |||
There was a problem hiding this comment.
Good you changed the assert into a coerce and let it do the coercion. Better than what I suggested.
|
|
||
| export const coerceDisplayInfo = allegedDisplayInfo => { | ||
| if (passStyleOf(allegedDisplayInfo) === REMOTE_STYLE) { | ||
| return harden({}); |
There was a problem hiding this comment.
This is too loose. Once you see it is a remotable, you should also assert that it is an empty object before returning a new empty object. Otherwise, an accidental passing of a substantive remotable object will silently become an empty object rather than rejected with a nice diagnostic.
There was a problem hiding this comment.
@erights How would you assert it is an empty object if it's a presence? I thought doing something like getting the ownPropertyNames wouldn't work.
katelynsills
force-pushed
the
119-decimal
branch
from
330086c to
a5fbbc6
Compare
…d is being miscategorized (for this purpose) as a presence
katelynsills
force-pushed
the
119-decimal
branch
from
a5fbbc6 to
eff15a4
Compare
This PR adds a
displayInfoobject to an ERTP issuerKit, including adecimalPlacesproperty in the same way that Ethereum uses theirdecimalsproperty. We intend for fungible digital assets to be represented as integers in the smallest unit. (In finance, this is often mills.) However, we want to display in a more user-friendly way.decimalPlacesis meant only for display, and informs the UI how many decimal places to move the decimal over to the left. For instance, for mills, one thousandth of a dollar, decimals would be 3.The downside is that this hardcodes a certain interpretation. For instance, it would hard-code a dollar as the standard representation instead of millions or thousands of dollars. However, the UI is free to ignore
decimals.One upside is that compared to something like a custom
toString, decimals is very safe. An attacker can't do much with the ability to choose an integer that will later be returned.Closes #119