Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve brand check #1000

Merged
merged 1 commit into from
Apr 27, 2020
Merged

Improve brand check #1000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/ERTP/src/amountMath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import harden from '@agoric/harden';
import { assert, details } from '@agoric/assert';

import { mustBeSameStructure, mustBeComparable } from '@agoric/same-structure';
import { mustBeComparable } from '@agoric/same-structure';
import mathHelpersLib from './mathHelpersLib';

/**
Expand Down Expand Up @@ -186,7 +186,10 @@ function makeAmountMath(brand, mathHelpersName) {
allegedBrand !== undefined,
details`alleged brand is undefined. Did you pass an extent rather than an amount?`,
);
mustBeSameStructure(brand, allegedBrand, 'Unrecognized brand');
assert(
brand === allegedBrand,
details`the brand in the allegedAmount in 'coerce' didn't match the amountMath brand`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have both brands. Can you print them? When I was debugging, this kind of thing has been very helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing to print. They are objects with a method (which we cannot access) and no properties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't they have getAllegedName()? That's enough to be useful.

Copy link
Contributor Author

@katelynsills katelynsills Apr 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the brand will likely be remote, so we can't call it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's too bad.

Any chance we can cheat and include the objects so the caller could catch the error and display it themselves? For once, it might be useful to include the opaque object in an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't pass access by throw. When you throw something, you know very little about who might catch it.

In any case, if you include the brands as substitution holes, e.g.

details`the brand in the allegedAmount in 'coerce' didn't match the amountMath brand: ${brand} vs ${allegedBrand}`,

this will throw an error with the message

`the brand in the allegedAmount in 'coerce' didn't match the amountMath brand: (an object) vs (an object)
See console for error data.`

This avoids even sending the stringification of these substitution values to the catcher by default, under the default assumption that we're willing to reveal the literal part of the details template literal, but only the typeof of their substitution value.

OTOH, to the console, we send the actual substitution values themselves, not a mere stringification of them. If you are in the privileged position of receiving what is sent to the console, for example because you provided the console, then you can receive these values directly. For these purposes, we consider this to be loosely equivalent to the privilege level of viewing the computation through a debugger.

Attn @michaelfig and I are working towards a reform in the distributed object system so that presences can carry description strings which will show up in the default builtin console behavior. So to see that, you merely need access to the textual output of the normal console, which is still a debug level of privilege.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erights, I read what you said above as indicating that stringifying the brands would be useless, and that including the objects would make them accessible to print on the console. OTOH, you also said they shouldn't be conveyed by a throw.
That sounds like including the objects in the details object would satisfy both objectives, since the console gets access to details, but obfuscates the values in the thrown message.
Am I misunderstanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting brand objects in details will not show anything useful currently. It will show something like { getAllegedName } (non-presence) or { } (presence)

);
// Will throw on inappropriate extent
return amountMath.make(extent);
},
Expand Down
9 changes: 6 additions & 3 deletions packages/ERTP/test/unitTests/mathHelpers/test-natMathHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import makeAmountMath from '../../../src/amountMath';

const mockBrand = harden({
isMyIssuer: () => false,
allegedName: () => 'mock',
getAllegedName: () => 'mock',
});

const amountMath = makeAmountMath(mockBrand, 'nat');
Expand Down Expand Up @@ -56,8 +56,11 @@ test('natMathHelpers', t => {
`coerce can take an amount`,
);
t.throws(
() => coerce(harden({ brand: {}, extent: 4 })),
/Unrecognized brand/,
() =>
coerce(
harden({ brand: { getAllegedName: () => 'somename' }, extent: 4 }),
),
/the brand in the allegedAmount in 'coerce' didn't match the amountMath brand/,
`coerce can't take the wrong brand`,
);
t.throws(
Expand Down
2 changes: 1 addition & 1 deletion packages/ERTP/test/unitTests/mathHelpers/test-setMathHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import makeAmountMath from '../../../src/amountMath';

const mockBrand = harden({
isMyIssuer: () => false,
allegedName: () => 'mock',
getAllegedName: () => 'mock',
});

const amountMath = makeAmountMath(mockBrand, 'set');
Expand Down
2 changes: 1 addition & 1 deletion packages/ERTP/test/unitTests/mathHelpers/test-strSetMathHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import makeAmountMath from '../../../src/amountMath';

const mockBrand = harden({
isMyIssuer: () => false,
allegedName: () => 'mock',
getAllegedName: () => 'mock',
});

const amountMath = makeAmountMath(mockBrand, 'strSet');
Expand Down
2 changes: 1 addition & 1 deletion packages/ERTP/test/unitTests/test-issuerObj.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ test('issuer.split bad amount', t => {
const payment = mint.mintPayment(amountMath.make(1000));
t.rejects(
_ => E(issuer).split(payment, otherUnitOps.make(10)),
/Unrecognized brand/,
/the brand in the allegedAmount in 'coerce' didn't match the amountMath brand/,
'throws for bad amount',
);
} catch (e) {
Expand Down