fix: Improve offer documentation

[gibson042]

...particularly to document offerArgs.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	c53f09e
Status:	✅  Deploy successful!
Preview URL:	https://4be18072.documentation-7tp.pages.dev
Branch Preview URL:	https://gibson-2023-11-improve-offer.documentation-7tp.pages.dev

View logs

[FUDCo]

A couple of minor nits, but this seems like a strict improvement.

[FUDCo]

Might be worth a mention in passing of some things that aren't passable, just to underscore that it's a meaningful concept.

[dckc]

Perhaps, but if so, let's put that in the Marshaling section.

I'd rather not have any novel info in the glossary. I'd like it to be more of an index. Each item should have a brief gloss plus a link to where the term is defined in context.

[Chris-Hibbert]

This is great! Lots of helpful changes here. I have a few suggestions, but mostly this is very good as it stands.

I'll bring up a local copy and verify links and visual formatting before approving.

[Chris-Hibbert]

Doesn't that HTML tag have to surround something to have an effect? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a says to use id instead.

stackoverflow thinks it's not okay for it to be empty.

[kriskowal]

I’ve found that this works, but the browser is not obliged to scroll farther than necessary to put the top of this marker above the fold. Capturing content obliges the browser to scroll to show as much of the content as fits.

[gibson042]

id is definitely the correct attribute (my muscle memory here is very old and outdated), but inner content is not necessary to establish what I know of as an "anchor link". This particular technique of using an empty a element before a Markdown section heading allows external links using an old target to still land in approximately the right place without suffering automatic slug generation issues that cause otherwise preferable Markdown like ## KeywordRecord <a id="amountkeywordrecord"></a> to render incorrectly in many implementations, e.g.:

--- expected
+++ actual
-<h2 id="keywordrecord">KeywordRecord <a id="amountkeywordrecord"></a></h2>
+<h2 id="keywordrecord-a-id-amountkeywordrecord">KeywordRecord <a id="amountkeywordrecord"></a></h2>

[Chris-Hibbert]

... what is being given, what must be received to satisfy offer safety, and an exit rule ...

I'd say "is being given" because zoe requires that the proposal match the payments, and the contract won't see the offer if they don't match. (and the invitation will be burned.

"what must be received to satisfy offer safety" because the user can want any arbitrary thing (subject to want patterns provided by the contract), and it's a valid offer. If the contract doesn't match the want, then the offer gets its give back.

Is there a reasonable place to describe want patterns? Do we talk about makeInvitation from the contract author's perspective?

[gibson042]

... what is being given, what must be received to satisfy offer safety, and an exit rule ...

I'd say "is being given" because zoe requires that the proposal match the payments, and the contract won't see the offer if they don't match. (and the invitation will be burned.

"what must be received to satisfy offer safety" because the user can want any arbitrary thing (subject to want patterns provided by the contract), and it's a valid offer. If the contract doesn't match the want, then the offer gets its give back.

Done.

Is there a reasonable place to describe want patterns?

I'll defer on this question, although Zoe Data Types and The Structure of Offers seem like reasonable locations for followup.

Do we talk about makeInvitation from the contract author's perspective?

It's mentioned at ZCF documentation and Writing and Installing a Contract, but they could go further.

[erights]

We do not yet support want patterns. Looking forward to it though!

[dckc]

The keyword record stuff doesn't look right.

[dckc]

ISTR some discussion of deprecating / removing creator invitation in the name of zoe durability.
@Chris-Hibbert @erights what became of that?

[Chris-Hibbert]

It's a hope, with no concrete plan behind it. Not worth covering in the docs at this point.

[kriskowal]

It might also be worth noting that startInstance is subsumed into startUpgradable. (It is, right?)

[Chris-Hibbert]

It might also be worth noting that startInstance is subsumed into startUpgradable. (It is, right?)

That doesn't sound familiar. I think you're misremembering something, though I don't know what.

[kriskowal]

I dug into this. The Agoric chain home object can provide startUpgradable as a thin wrapper for E(zoe).startInstance. It’s provided by vats/src/core/basic-behaviors.js. This is why we were unable to find startInstance in the core proposal in the dapp-game-places template during the hackathon. We eventually found startUpgradable and from there were able to figure out how to thread offerArgs thru the level of indirection.

[dckc]

startUpgradable is a power from the bootstrap environment; it's available to core-evals, but it's not part of the Zoe API.

The purpose of startUpgradable is somewhat relevant, though: it's to make sure we don't lose admin facets needed for upgrade.

Do we tell folks in the Zoe docs that startInstance() also returns an admin facet, and if you lose it, you will NEVER BE ABLE TO UPGRADE?

[dckc]

startUpgradable should be covered in docs for bootstrap powers. Those are pretty thin. That's probably worth a separate issue...

[erights]

ISTR some discussion of deprecating / removing creator invitation in the name of zoe durability.
@Chris-Hibbert @erights what became of that?

It's a hope, with no concrete plan behind it. Not worth covering in the docs at this point.

I think we should explicitly state that it is deprecated. Whether we will ever be able to remove it remains to be seen, but I'm hopeful.

[dckc]

I looked for a relevant issue and found:

• deprecate and remove use of creatorInvitation as a legal return value from contracts agoric-sdk#5775

But it's one of those "do X" issues that doesn't say why do X... other than

That functionality can be supplied from the creatorFacet.

[erights]

Thanks for finding that. In light of @samsiegart 's two comments at Agoric/agoric-sdk#5775 (comment) , I withdraw the request to emphasize the deprecation of creator invitation until Sam's question is resolved.

[dckc]

Perhaps, but if so, let's put that in the Marshaling section.

I'd rather not have any novel info in the glossary. I'd like it to be more of an index. Each item should have a brief gloss plus a link to where the term is defined in context.

[dckc]

Is it reasonable to assume that our audience is familiar with the use of "record" to mean "object"?

When we talk about things like a keyword record or an issuer record, there's more context. But here, there is precious little.

[Chris-Hibbert]

I thought that was official JavaScript terminology.

[kriskowal]

I believe @erights coined record and in his interpretation, record means const-object-as-struct. The TC-39 Records and Tuples proposal uses the word to mean new-primitive-as-immutable-struct. TypeScript’s Record type suggests object-as-mutable-dictionary. I believe offerArgs is a const-object-as-dictionary. I am distinguishing “const” from “hardened” to mean: the informal contract is that this method will not mutate it and it consequently may be hardened.

It might be best to just say Object here.

[kriskowal]

@gibson042 This is great. We saw places where offerArgs was typed unknown and others where it was typed Object. I think we need to also converge on the latter in the code.

[gibson042]

Updated.

To pass additional arguments to the offerHandler contract code associated with the
invitation, send them in an offerArgs CopyRecord.

[dckc]

That example seems weird. I don't believe anything in our Record<Keyword, Brand | bigint>. Did you mean to exhibit a Record<Keyword, Amount<'nat'>>? aka a Record<Keyword, { brand: Brand<'nat'>, value: NatValue }>

I wonder if it's useful to generalize to KeywordRecord at all. If so, we might as well lean into TypeScript notation: Record<Keyword, T>

[gibson042]

That example seems weird. I don't believe anything in our Record<Keyword, Brand | bigint>. Did you mean to exhibit a Record<Keyword, Amount<'nat'>>? aka a Record<Keyword, { brand: Brand<'nat'>, value: NatValue }>

No, for that first example I specifically wanted a KeywordRecord that had heterogeneous value types and so was not an AmountKeywordRecord/PaymentKeywordRecord/etc. But if you think this one is confusing, perhaps it could drift even further away from our flavor of DeFi?

Suggested change

	is a **[Keyword](#keyword)**, such as `harden({ Currency: quatloosBrand, CurrencyUnit: 100n })`.
	is a **[Keyword](#keyword)**, such as `harden({ Remotable: myMap, SupportedMethods: ['delete', 'get', 'has', 'set'] })`.

I wonder if it's useful to generalize to KeywordRecord at all. If so, we might as well lean into TypeScript notation: Record<Keyword, T>

I thought about that and actually started with something similar, but ultimately decided not to because a) the site already uses AmountKeywordRecord and PaymentKeywordRecord, and b) we don't explain use of TypeScript syntax and AFAICT don't expect developers to know it. But a thought did occur to me in typing the last response—what would you think about using it to help explain the concept for people who are familiar with it?

KeywordRecord

A KeywordRecord is a CopyRecord in which every property name
is a Keyword, such as harden({ Currency: quatloosBrand, CurrencyUnit: 100n }).
Subtypes further constrain property values (for example, an
AmountKeywordRecord is a KeywordRecord<Amount> in which every value is an
Amount and a
PaymentKeywordRecord is a KeywordRecord<Payment> in which every value is a
Payment).

[dckc]

... for that first example I specifically wanted a KeywordRecord that had heterogeneous value types ...

Such an example is a distraction, no? There is no such data type in the Zoe API.

This is the core of my request for changes.

[Chris-Hibbert]

I agree. There are no KeywordRecords with heterogeneous value types, and I don't think we're likely to add them.

[gibson042]

Okay, updated.

A KeywordRecord is a CopyRecord in which every property name
is a Keyword, such as harden({ Asset: moolaIssuer, Bid: simoleanIssuer }).

[erights]

KeywordRecord that had heterogeneous value types

Please don't. None of our keyword records are heterogeneous, and likely none ever will be, since the point of keywords as names is to support lookup by [] user-defined names, and therefore computed property names.

[dckc]

comment: not having these kept in sync with agoric-sdk by machine is just maddening.

• Sync Zoe JSDoc with API docs agoric-sdk#4291

[erights]

comment: not having these kept in sync with agoric-sdk by machine is just maddening

Indeed!

[dckc]

Suggested change

	**paymentKeywordRecord** must be either `undefined` or a **[PaymentKeywordRecord](./zoe-data-types.md#keywordrecord)**
	**paymentKeywordRecord** must be either `undefined` or a **[PaymentPKeywordRecord](./zoe-data-types.md#keywordrecord)**

https://github.com/Agoric/agoric-sdk/blob/0160e2fcb277340db2e3d25fca4c79af59b7e87c/packages/zoe/src/zoeService/types.js#L154

[Chris-Hibbert]

Looks like you're right. I consider the typeGuards definitive. Thanks for the correction.

[kriskowal]

Thank you, suggestions only.

[kriskowal]

It might also be worth noting that startInstance is subsumed into startUpgradable. (It is, right?)

[kriskowal]

Suggested change

	A *passable* is something that can be sent to and from remote objects.
	A *passable* value is a JavaScript value that can be sent to and from remote objects.

[kriskowal]

I believe @erights coined record and in his interpretation, record means const-object-as-struct. The TC-39 Records and Tuples proposal uses the word to mean new-primitive-as-immutable-struct. TypeScript’s Record type suggests object-as-mutable-dictionary. I believe offerArgs is a const-object-as-dictionary. I am distinguishing “const” from “hardened” to mean: the informal contract is that this method will not mutate it and it consequently may be hardened.

It might be best to just say Object here.

[kriskowal]

@gibson042 This is great. We saw places where offerArgs was typed unknown and others where it was typed Object. I think we need to also converge on the latter in the code.

[kriskowal]

I’ve found that this works, but the browser is not obliged to scroll farther than necessary to put the top of this marker above the fold. Capturing content obliges the browser to scroll to show as much of the content as fits.

[dckc]

keyword stuff is addressed to my satisfaction

[erights]

Just comments so far, only on the README

[erights]

ISTR some discussion of deprecating / removing creator invitation in the name of zoe durability.
@Chris-Hibbert @erights what became of that?

It's a hope, with no concrete plan behind it. Not worth covering in the docs at this point.

I think we should explicitly state that it is deprecated. Whether we will ever be able to remove it remains to be seen, but I'm hopeful.

[erights]

Again, I think we should deemphasize and deprecate creator invitations.

[erights]

Likewise withdrawn until Sam's question is resolved.

[erights]

Possible misunderstanding that offer safety means you'll get what you said you want. At least make "offer safety" and explicit link to that glossary entry. Avoiding the possible misunderstanding would be even better.

[erights]

A few more. But I clearly won't finish tonight.

Btw, I am not trying to check or follow any links, as I cannot do so simply in the markdown preview mode.

[erights]

What's an agent? Can we avoid this terminology completely please?

Sigh. To get rid of "agent" you'd also need to fix the diagram.

[erights]

That diagram is nice!

[erights]

Worse here. Is the parameter name objectWithMethods or remotable?

Probably worth emphasizing that the objectsWithMethods MUST not be hardened or even frozen at the time of the Far call.

[erights]

some more. not done yet.

[erights]

below

The *gains*' **[Keywords](./zoe-data-types.md#keyword)** are in that **seat**'s namespace.

What does this mean?

[gibson042]

I think your guess is as good as mine... maybe referring to use of Keywords defined by the contract?

[Chris-Hibbert]

That seems like a good interpretation, but we don't refer to keywords as a namespace anywhere else, so better to recast this in more familiar terminology.

[gibson042]

My best-effort rewording:

aZCFMint.mintGains(gains, zcfSeat?)

• gains: AmountKeywordRecord
• zcfSeat: ZCFSeat - Optional.
• Returns: ZCFSeat

All amounts in gains must be of this ZCFMint's Brand
and the gains' Keywords should be defined by the contract instance in which zcfSeat is participating.
If zcfSeat is not provided, a new seat is used.
Mints the gains Amount of assets and adds them to zcfSeat's Allocation, then returns zcfSeat.

aZCFMint.burnLosses(losses, zcfSeat)

• losses: AmountKeywordRecord
• zcfSeat: ZCFSeat
• Returns: None

All amounts in losses must be of this ZCFMint's Brand
and the losses' Keywords must be defined by the contract instance in which zcfSeat is participating.
Subtracts losses from zcfSeat's Allocation, then
burns that amount of assets from the pooled Purse.

[erights]

below

then burn that amount of assets from the pooled Purse.

This is true and important. But would a reader have any idea what this pooled Purse is?

[Chris-Hibbert]

How about changing then burns that amount of assets from the pooled Purse. to

then burns that amount of assets from the amount escrowed by Zoe for this contract instance.

[erights]

@Chris-Hibbert , are they actually burned? That surprises me.

[Chris-Hibbert]

Your instincts are correct. If the invitation is blocked, we Fail out of the offer() call and don't get to the immediately following burnInvitation().

    !instanceAdmin.isBlocked(description) ||
      Fail`not accepting offer with description ${q(description)}`;
    const { invitationHandle } = await burnInvitation(
      invitationIssuer,
      invitation,
    );

[Chris-Hibbert]

Please change "will be burned and not processed if passed to E(Zoe).offer()" to "will not be processed if passed to E(Zoe).offer(). Instead, an exception will be thrown.".

[Chris-Hibbert]

Btw, I am not trying to check or follow any links, as I cannot do so simply in the markdown preview mode.

I am planning to pull down the whole PR and do the local build and review for link consistency, but I'm waiting for the changes to settle down.

[erights]

Likewise withdrawn until Sam's question is resolved.

[erights]

What about Errors?

[erights]

Since payouts.Asset may be a promise for a payment, and since purse.deposit demands a non-promise for its payment argument, this example is incorrect or at best misleading.

[erights]

This review pass complete. Over to you ;)

[gibson042]

Thanks for the astonishingly thorough review, @erights! I believe I've addressed all feedback.

[Chris-Hibbert]

Extremely close. A few last-second nits.

[Chris-Hibbert]

I keep reading this and trying to re-word to "by making offers to it". But I think the it in "with it" is the invitation and the it in "to it" is the contract. Would you mind rewording? Perhaps "by using the invitation as the first argument to zoe.offer()".

[erights]

Those pronouns. Dereference them!

[Chris-Hibbert]

How about changing then burns that amount of assets from the pooled Purse. to

then burns that amount of assets from the amount escrowed by Zoe for this contract instance.

[Chris-Hibbert]

Woops! customProperties should be customDetails. (Please fix above) The description here is garbled.

Suggested change

	**description** is a required string describing the **Invitation**,
	and should include whatever information is needed for a potential recipient of the **Invitation**
	to know what they are getting in the optional *customProperties* argument, which is
	put in the **Invitation**'s **value**.
	to know what they are getting in the optional **customProperties** argument, which is
	put in the **Invitation**'s **amount**.
	Each one should be a string literal that is unique within its contract and
	used only as the argument to make invitations of a particular kind.
	**description** is a required string describing the role of this **Invitation**,
	and should include whatever information is needed for a potential recipient of the **Invitation** to distinguish amoung this contract's invitations. Each description
	should be a string literal that is unique within its contract.
	The (optional) **customDetails** argument is included in the **Invitation**'s
	**amount** and not otherwise relied on by Zoe.

[gibson042]

I see that proposalShape lacks a description; would you care to suggest some text for it as well?

[Chris-Hibbert]

Oh, man, that is not a small topic, and I don't see it touched on anywhere in these docs. The only reference material for it that I can find is in endo, and it is far from adequate as developer documentation.

I would say "a Pattern[ref] describing the required and allowed components of the proposal. Proposals that don't match the pattern will be rejected by Zoe without interacting with the contract." I won't claim that's adequate, but to say any more would take a week.

[erights]

I still think you need to explicitly say that they are unique to a static call site within this contract. Or better, within this contract's source text. Otherwise, someone might misread to think they need to be unique per dynamic call to makeInstance.

[Chris-Hibbert]

@erights's comment applies to the description

[erights]

Yes. Only the description string. Thx.

[Chris-Hibbert]

Please change "will be burned and not processed if passed to E(Zoe).offer()" to "will not be processed if passed to E(Zoe).offer(). Instead, an exception will be thrown.".

[Chris-Hibbert]

I don't think it's a different type of issuer. How about

Suggested change

	The **InvitationIssuer** is a special type of **[Issuer](/reference/ertp-api/issuer.md)**.
	Zoe has a single **InvitationIssuer** for its entire lifetime. All **Invitations** come from the
	The **InvitationIssuer** is an **[Issuer](/reference/ertp-api/issuer.md)** that plays a
	special role throughout Zoe. Zoe has a single **InvitationIssuer** for its entire
	lifetime. All **Invitations** come from the **[Mint](/reference/ertp-api/mint.md)**
	associated with the Zoe instance's **InvitationIssuer**.
	The issuer is publicly available (via `E(Zoe).getInvitationIssuer()`), so the ability
	to validate invitations is widely available.

[Chris-Hibbert]

Suggested change

	what is expected in exchange (protected by offer safety), and
	what is desired in exchange (protected by offer safety), and

[Chris-Hibbert]

Suggested change

	Every **Keyword** in **give** must also have a corresponding **payment**.
	Every **Keyword** in **give** must have a corresponding **payment**.

[gibson042]

@Chris-Hibbert updated with your suggestions.

[gibson042]

Thanks all!

[gibson042]

@erights Please remove or replace your "changes requested" review so this can merge.

[erights]

LGTM

Please do look at all unresolved conversations to see if there's anything unaddressed before merging. Otherwise good to go. Awesome improvements, thanks!