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.

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

Restrict "burn" to describing operations that destroy assets #655

Merged
merged 12 commits into from
Apr 11, 2022

Conversation

gibson042
Copy link
Member

Fixes #654

Also includes general consistency/cleanup of affected pages. Probably best reviewed commit-by-commit.

@gibson042 gibson042 requested a review from erights April 2, 2022 03:05
@gibson042 gibson042 self-assigned this Apr 2, 2022
@netlify
Copy link

netlify bot commented Apr 2, 2022

Deploy Preview for agoric-docs ready!

Name Link
🔨 Latest commit 2b6b500
🔍 Latest deploy log https://app.netlify.com/sites/agoric-docs/deploys/625474072cdfcc0008e85fc2
😎 Deploy Preview https://deploy-preview-655--agoric-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -89,8 +89,7 @@ const quatloosIssuerAllegedName = quatloosIssuer.getAllegedName();
## `issuer.getAssetKind()`
- Returns: `{AssetKind}`

Get the kind of this `issuer`'s asset. It returns one of
`AssetKind.NAT` (`nat`) or `AssetKind.SET` (`set`).
Return the kind of the `issuer`'s asset; either `AssetKind.NAT` ("nat") or `AssetKind.SET` ("set").
Copy link
Member Author

Choose a reason for hiding this comment

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

To anticipate a request, I think we'll want to document AssetKind.COPY_SET and AssetKind.COPY_BAG here, as was done in 85fc794#diff-567699eff5882f854f56340cee0537815d3e363b61e098208212dc172190dd3dR43 . But I think that belongs in a followup.

Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM. Awesome to see these cleanups! Thanks much!!!

Comment on lines 25 to 26
An `issuer` uniquely identifies its `brand`. A `brand` **unreliably** identifies
its `issuer`. If `brand` B claims its `issuer` is A, but A doesn't agree
Copy link
Member

Choose a reason for hiding this comment

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

That's not quite right. Either or both the issuer and the brand may be ill behaved, which we cannot in general defend against. They may also represent different interests. The issuer may claim that someone else's brand is its own brand. The brand may agree that someone else's issuer is its own issuer. What we need to verify, and what this protocol enables us to verify, is that this issuer and this brand each endorse the other. We need mutual agreement. This tells us that they at least represent one interest. Zoe does in fact verify this when she registers a new issuer.

main/ertp/api/issuer.md Show resolved Hide resolved
@@ -66,7 +66,7 @@ const combinedProperty = AmountMath.make(propertyTitleBrand, ['1292826', '102839
## `issuer.getAllegedName()`
- Returns: `{allegedName}`

Returns the `allegedName` for this `issuer`.
Return the `allegedName` for the `issuer` (the non-trusted human-readable name of its associated `brand`).
Copy link
Member

Choose a reason for hiding this comment

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

I think it's work having an example here, like how anyone can create a new issuer whose allegedName is BTC.

@@ -89,8 +89,7 @@ const quatloosIssuerAllegedName = quatloosIssuer.getAllegedName();
## `issuer.getAssetKind()`
- Returns: `{AssetKind}`

Get the kind of this `issuer`'s asset. It returns one of
`AssetKind.NAT` (`nat`) or `AssetKind.SET` (`set`).
Return the kind of the `issuer`'s asset; either `AssetKind.NAT` ("nat") or `AssetKind.SET` ("set").
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue?

Comment on lines 112 to 114
Describe the `payment`'s balance as an Amount. Because `payment` cannot
be trusted to provide its own true value, `issuer` must be used to validate
the `payment`'s `brand` and report how much it contains.
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this explanation. The mistrust we have in payments is not due to some intrinsic feature of the payment. It is due to the expected use pattern, where Bob might receive one from Alice, who he doesn't trust. Since Bob doesn't trust Alice, he must consider the possibility that Alice might have sent him something else in the payment position.

@@ -74,7 +74,7 @@ const checkNotifier = async () => {
- `optAmount` `{Amount}` - Optional.
- Returns: `{Amount}`

Deposit all the contents of `payment` into this `purse`, returning an `amount` describing the
Deposit all the contents of `payment` into `purse`, returning an `amount` describing the
`payment`'s digital assets (i.e. the deposit amount). If the optional argument `optAmount` does not equal the balance of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`payment`'s digital assets (i.e. the deposit amount). If the optional argument `optAmount` does not equal the balance of
`payment`'s digital assets (i.e. the deposited amount). If the optional argument `optAmount` does not equal the balance of

@@ -74,7 +74,7 @@ const checkNotifier = async () => {
- `optAmount` `{Amount}` - Optional.
- Returns: `{Amount}`

Deposit all the contents of `payment` into this `purse`, returning an `amount` describing the
Deposit all the contents of `payment` into `purse`, returning an `amount` describing the
`payment`'s digital assets (i.e. the deposit amount). If the optional argument `optAmount` does not equal the balance of
`payment`, or if `payment` is an unresolved promise, it throws 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.

Important:

Suggested change
`payment`, or if `payment` is an unresolved promise, it throws an error.
`payment`, or if `payment` is a promise, it throws an error.

- <<< @/snippets/ertp/guide/test-issuers-and-mints.js#getAllegedName
- [`issuer.getAssetKind()`](/ertp/api/issuer.md#issuer-getassetkind)
- Get the kind of asset for this `issuer`, either `AssetKind.NAT` (`nat`),
or `AssetKind.SET` (`set`).
- Return the kind of the `issuer`'s asset; either `AssetKind.NAT` ("nat") or `AssetKind.SET` ("set").
Copy link
Member

Choose a reason for hiding this comment

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

Here too ;)

- Returns a new empty `purse` for the `brand` associated with the `issuer`. The `purse` only accepts valid
deposits of its associated `brand`, so you can retroactively identify a valid `payment` of that `brand`
by successfully depositing it into this `purse`.
- Make and return an empty `purse` that holds assets of the `brand` associated with the `issuer`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Make and return an empty `purse` that holds assets of the `brand` associated with the `issuer`.
- Make and return an empty `purse` for holding assets of the `brand` associated with the `issuer`.

Since it is empty, I think this is slightly better.

@@ -135,11 +86,11 @@ API Reference](/ertp/api/#ertp-api).
the `mint`, `brand` or `purse` are associated with it, then they're invalid. These methods help you find
the right `issuer`, but aren't authoritative.
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to rewrite this paragraph!

gibson042 added a commit to gibson042/Agoric-documentation that referenced this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Burn" should be restricted to describing operations that destroy assets
2 participants