Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 207 forks source link

Stuff I Learned at The Hackathon #1282

Open FUDCo opened 4 years ago

FUDCo commented 4 years ago

Stuff I Learned At The Hackathon

-- Documentation is weirdly linked, in that there appears to be structured with a series of links on each page the present a flow from one page to the next, but if you just read following those links in order you will miss a bunch of stuff. In general, it's hard to find things in our documentation if you don't already know where they are.

-- There is a large number of interlocking abstractions in Zoe (specificaly) and the dapp framework (more generally), and the relationships between them are non-obvious but not entirely explained. In particular, even though a lot of the specific APIs are documented, the relationships between the components remain obscure. The best source of education for me was walking through the dapp-encouragment contract code with Chris, with me reverse engineering what must going on and Chris telling me when I was right and when I was wrong in what I had figured out. I ended up with a reasonable understanding of the contract portion of the dapp (enough that I could code the Agorable breeding transaction and get it working pretty much on my own), but I still don't have nearly that level of understanding of the API portion and I remain relatively clueless about how the UI portion interacts with those.

In particular, the encouagement dapp contract names things in ways that aren't so much part of the API spec as coding choices made by the original authors, and these sometimes lead me astray. In particular, it wasn't obvious what an "invite" is: I knew it was a capability to gain access to a contract, but what specifically it gains access to wasn't really that clear. It turns out that what it enables you to do is invoke a particular entry point in the contract code via the mechanism of submiting an offer to Zoe, providing the invite as one of the parameters. This is quite indirect. More confusing was that the piece of contract code that gets invoked via this path is called an "offer hook". I don't think we really have a name for this kind of entry point (the thing that the offer hook is associated with) -- I've heard people variously call it a role or a seat or a few other words, none of which really have the flavor of "a specific, indentified point at which traffic comes in from the world outside the contract". The parameter to the offer hook function is an "offer handle", which is not actually the offer but a key for looking up the offer -- and the "offer" is not really an offer but a bundle of offer terms that represents some party's current position in the contract (which could have changed over the life of the contract from what was originally in the offer that was submitted). The "offer" is really more of a kind of account record, representing the state of an ephemeral account that was birthed by the submission of an offer but which has a life of its own after that.

-- I still don't have any idea what value, if any, is provided by the keyword record. At this point I'm about 90% convinced that this abstraction could be dispensed with entirely with no loss of generality, expressiveness, or safety. I think I pretty much managed to convince Chris of this as I was trying to work through what it was for. In particular, the brandKeywordRecord was largely an inconvenience. It was used two places in the encouragment contract (in addition to the place where it is retrieved via the API) and as I adapted encouragment code for Agorables I ended up refactoring my code to eliminate one of these uses, with the resulting code being clearer.

We start by fetching it:

    const { brandKeywordRecord } = zcf.getInstanceRecord();

then use it to extract the amount math object for the fee:

    const feeAmountMath = zcf.getAmountMath(brandKeywordRecord.Fee);

(By the way, I eventually shortened feeAmountMath to feeMath, which made things noticeably more readable.)

Then, considerably later in the code, we needed to extract the various things that were in one of the offers, which resulted in:

    const {
      Fee: hostFeeAllocation,
      Monster: hostMonsterAllocation,
    } = zcf.getCurrentAllocation(
      hostOfferHandle,
      brandKeywordRecord,
    );

which is quite a mouthful. In the process of debugging, we discovered that we shouldn't be putting the Monster in the host offer (at least, not in its initial state), so that eliminated one part of this incantation:

    const {
      Fee: hostFeeAllocation,
    } = zcf.getCurrentAllocation(
      hostOfferHandle,
      brandKeywordRecord,
    );

Call this form #1; it'll come up again shortly.

At the point this code was initially birthed, this was all in the offer hook for the "buy" operation. However, at the point where we introduced the "breed" operation, this introduced another offer hook. It turned out that buy and breed operations shared a fair bit of common mechanism. In particular, both wanted to start by collecting a fee. But to factor this out into a function of its own required passing around the brandKeywordRecord, which was icky. Then I noticed a fragment that extracted the fee amount directly:

    const userFeeAllocation = zcf.getCurrentAllocation(offerHandle).Fee;

Call this form #2 (ironicaly, this was on the line immediately prior to fetching the brandKeywordRecord)

Hey, what's going on here? What's the different between form #1 and form #2? (Aside from one being six lines and the other being one line, that is.) Well, it turns out that if you have an offer that contains no Fee property, form #2 will yield a value of undefined whereas form #1 will yield a value of 0, and in the context where this is happening it turns out we need the semantics of form #1. There were two problems with this. First, we actually need two of these, one each for the host offer and the client offer. Twelve lines is even worse that six lines. Second, the most natural place to do this is before we've obtained the brandKeywordRecord.

Well, I ended up writing a little function:

  function getFeeAllocation(offer) {
    let result = zcf.getCurrentAllocation(offer).Fee;
    if (!result) {
      result = feeMath.make(0);
    };
    return result;
  }

This enables extraction of the fee value in one line where needed, without recourse to the brandKeywordRecord at all.

Our other use of the brandKeywordRecord was to get the feeMath object from the instance record (note that we didn't need the brandKeywordRecord to get the monsterMath object because we're generating the Monster issuer inside the contract itself and so we got the math object back directly inside the result of produceIssuer). But having noticed that this is the only remaining use, I realized I could move the amount math extraction out of the offer hook functions and into their shared outer scope (this had always been possible, it just wasn't obvious).

-- "Allocation" turns out to be another one of those slightly confusing distinctions. The way I ended up thinking of it, an allocation is to an offer as an amount is to a payment. Allocation and offer are both collections of typed quantities, whereas amount and payment are both singular typed quantities. Orthogonally, allocation and amount are both mentions, whereas offer and payment are both uses:

collection singular
use Offer Payment
mention Allocation Amount

Putting them in a tidy 2x2 matrix like this makes it easier to present the distinctions, but now it makes me wonder if different names for the abstractions might somehow better reflect the two dimensions. I also note that the collection things are collections of the singular things, though I'm not sure that's actually as useful to understand.

Related to this, I kept stubbing my toe on the design of the reallocate function. To my sensibilities, the parameters are rotated 90 degrees to what they should be. That is, the parameters are two parallel sequences of offers and allocations, whereas I repeatedly kept wanting to read each parameter as an offer/allocation pair. This confusion is especially easy in the common case of having two offers to be reallocated, so that it's a 2x2 matrix. The fascinating thing was that I kept repeatedly making this mistake even after I knew what was there and was consciously trying not to fall into the trap. Dean's reaction upon my telling of this was "that's why I never use reallocate; use trade instead". So I looked at trade and it's much clearer, except (a) it's super verbose and (b) it's only designed to work between two participants (albeit this is a super common case -- by far the most common, I'd wager). (Also, the verbosity does look like it pays for itself by letting you skip packing stuff into allocations, which is nice.) (Also also: I don't understand why one hardens the parameters to reallocate but not to trade. Since reallocate is part of Zoe you're already dependent on it, so harden here just seems to be gilding the lilly.)

The design of trade, combined with the pattern I fell into of early extraction of individual values from the offer, makes me wonder if the allocation abstraction could be dispensed with entirely.

-- We eventually renamed buyHook to doBuy, another clarity improvement -- we actually grappled with what to call this for some time; one obvious possiblity was that since the function was handling the offer, we could change "hook" to "handle", as in handleBuy, but there's the problem that we'd then be using the word "handle" with two different meanings depending on what part of speech was intended: the verb form, meaning "take care of stuff", and the noun form, meaning "a grasp onto some stuff" (e.g., offerHandle). But the name buyHandle could be interpreted either way; only the knowledge that it's a function name would sort things out. We decided to stick with the established use of the word in the noun form and do something else, hence "do" which is unambiguously a verb.)

-- The fact that the offer handle is opaque token rather than an object means that you're constantly doing extra work, because you have to do everything indirectly. Consider the example above of extracting the fee:

   zcf.getCurrentAllocation(offerHandle).Fee

It feels like it would be nicer to just be able to say:

   offerHandle.Fee

This leaves zcf out of the picture, but since Zoe gave me the offer handle to begin with I don't know why it couldn't give it to me in the form of an object with a useful API.

warner commented 4 years ago

My writeup:

Invariant/Error Reporting

My biggest struggle with the hackathon was learning small syntax details about the API by trial-and-error. We have a lot of invariant assertions in the code, both for security purposes and for general sanity checks. However the exceptions they raise don't contain enough information for a developer to figure out what when wrong. It might just be my personal style, but I frequently learn a new system by making calls that I know are probably wrong, and then looking for error messages that will tell me what to do better.

I made a bunch of mistakes which provoked error messages that weren't easy to learn from:

In many of these cases, the error was raised by code that was checking the safety of a given offer, however it couldn't tell me which offer was considered unsafe, nor what constraints it was being compared against. The safety checks were factored into small pieces (loop over all Issuers named in an offer, use each to look up the corresponding proposed allocation, extract the amounts from both, then finally compare the amounts). The small piece that noticed the problem (amount comparison) didn't even know the name of the thing being compared, so its Error object was nearly empty.

If the only reason these checks fail is due to deliberate malice, then we aren't obligated to help that attacker understand what they did wrong. But many beginner developers are indistinguishable from sophisticated attackers.

When we improve the logging system, I'm looking forward to this code being able to register some sort of context ("I'm now evaluating the safety of allocation (alice gets 1 Moola, bob gets 1 Painting) against alice's Offer (requiring 2 Moola)"), so that when the error is raised, somebody (at least me, the developer watching stderr) can figure out where the problem lies. This was compounded by helper functions (like trade, or the burn that we wrote) which created new offers internally, such that the problematic offer wasn't even one created by the code we were writing.

Owning assets vs telling the owner what to do with them

Our system is kind of unique in that code can own assets, or it might be in a position to acquire assets eventually, or it might have (limited) control over the allocation of assets, all at the same time. I imagine folks getting frustrated because they can't just write an API handler which moves tokens from one bucket to another, but must instead phrase all such movements as Offers and stuff.

The token-economy dapp has a lot of place like the Vault, which can grant loans of a stablecoin in exchange for collateral of some kind. The Vault mints the stablecoin, and (briefly) owns it outright before giving it to the borrower. But the collateral it receives is merely an offer (Zoe tells the Vault that the collateral has been escrowed). So there's a weird intermediate step where it's acting as the allocation-deciding contract (with a set of offers, one of which is from the borrower), and also as a primary agent (minting new stablecoins) which then makes a new offer (stablecoins for collateral) to Zoe, and that offer handle then shows up at the contract (because it must be matched to the borrower's offer). It makes coins, puts them in an offer, and then gives the offer to itself.

That felt tricky, and I think we need to find a way to make it look non-tricky. We might document the different levels-of-tricky separately: one set of docs for contracts like auction algorithms which never own the assets they're allocating (the auctioneer doesn't own the painting, doesn't touch the money, they only tell the cashier how to allocate everything). Then a second set for the more complicated case where the contract handler is also an active participant.

external API is defined by make-invitation function

Each piece of a contract has an API surface that's defined by two functions, the first to create an invitation for something, and the second to implement the something (specifically the "hook" which is associated with that particular invitation).

I now understand why it has to be this way (clients rely upon Zoe, not the invitation provider), but it was strange to me. Most RPC-like systems just define a set of API functions, and then clients call them. I'm not used to working with a system where there's a secondary layer of protection that I must use to verify/sanitize everything I get back from the API calls.

I'm not sure what might help here. Maybe an early section in the docs that shows the difference between traditional RPC programming and what the Zoe world looks like ("if you would normally do X(), here you'll do makeXInvite() and then write a handler for those invitations"). There might be a different pattern to use in the definition process which would make it feel more natural, although I'll admit the zcf.makeInvitation call is about as simple as I can currently imagine.

"hooks" are really "handlers"

This made it hard for me to read the code sometimes. I heard Dean say that our use of "hook" follows what the React world does, but in my mind, a "hook" is a way to intercept, observe, permit/deny, and maybe modify the normal processing of some event. It usually appears as a register(func)/unregister API, or a variable (frequently a list) to which you can add a function. The process normally works a certain way, but you can use the hook to change it. Hooks are optional: the process works fine (and precisely as documented) when none have been added.

In contrast, a "handler" is the exactly-one function which handles a given kind of request. The function is provided at the same time the request or request type is created/registered.

In my vocabulary, zcf.makeInvitation takes a handler, not a hook.

checkHook() wrapper could be first-class

All of our APIs used the same pattern: zcf.makeInvitation(checkHook(fooHook, expected), 'meaningless string'). I know functions returning functions is all the rage, but this expected functionality seemed to be so common that I wouldn't mind seeing it be part of the first-class API: zcf.makeInvitation(fooHook, expected, 'string').

Chris-Hibbert commented 4 years ago

My own observations, which are mostly independent of Chip's above, even though we worked together a lot.

Wallet API

Pre-hackathon, I had made modifications and additions to the messages sent to and from the Wallet. It was always a serious challenge and an unrewarding task. Part of the issue is the lack of documentation, but it also seems like a convoluted mechanism, requiring correlated modifications in four or five disparate places. After working with Kowbert, I suspect that the system is simpler than it appears.

While working on the agorables project, Kris had stripped out everything we didn't need while converting the encouragement dapp to the framework we'd use for Agorables. When I wanted to add the wiring to support breeding requests to the UI, I followed the model provided by an earlier commit from Kris for buying an Agorable. It nly seemed to require changes in two places, and the entry point in the contract that we had added earlier was being invoked. My guess is that we only needed to send the messages one direction, and the examples I had tried to follow previous were sending messages and responses or acknowledgements, making the whole thing look more complex than necessary for some simple cases.

It would still be better to support capTP rather than this text-based protocol that requires explicit handlers for each message, but if we're not going to make that change, the documentation on where to make changes for particular tasks might not be that hard to write.

Keyword based records

javascript may have decent support for dealing with keyword-based records as JSON data, but combined with eslint's formatting requirements, it gets verbose and hard to work with. I wondered whether some of the places where we have a parameter that is shaped like { give: { Payment: p, Fee: f }, want: { Goods: g } } wouldn't be improved by turning it into positional parameters. I don't have specific suggestions, but it seems worth considering if we want to simplify.

General naming

functions and parameters in the API and doc are often named from Zoe's point of view rather than that of the contract. We should name everything from the exterior viewpoint now that we have things working, as that's where the learning hurdle is most costly to us.

dtribble commented 4 years ago

Re hook/handler naming: After reading Chip and Brian's comments, the common naming paradigm for handlers in JS is "on" so perhaps onBuy rather than doBuy or buyHook?

warner commented 4 years ago

Re hook/handler naming: After reading Chip and Brian's comments, the common naming paradigm for handlers in JS is "on" so perhaps onBuy rather than doBuy or buyHook?

Can invitations be used multiple times? If so, yeah, that sounds good. If not.. well, it's better than "hook", but implies (to me) multiple invocations, which isn't quite right.

erights commented 4 years ago

Only once.

FUDCo commented 4 years ago

Consider the following (extended) fragment of code from the agorables contract test:

  const monsterIssuer = E(publicAPI).getMonsterIssuer();

  async function buyMonster(payment) {
    const buyInvite = await E(publicAPI).makeBuyInvite();
    const buyProposal = harden({ give: { Fee: bucks5 } });
    const buyKeywordRecord = harden({ Fee: payment });
    const { payout: buyPayoutP } = await E(zoe).offer(buyInvite, buyProposal, buyKeywordRecord);
    const buyPayout = await buyPayoutP;
    const { Fee: feeP, Monster: buyMonsterPaymentP } = buyPayout;
    return buyMonsterPaymentP;
  }

  const monster1Payment = await buyMonster(bucksPayment1);
  const monster1Amount = await E(monsterIssuer).getAmountOf(monster1Payment);
  const [ monster1Serial ] = monster1Amount.extent;
  const monster1Genome = await E(privateAPI).genome(monster1Serial);
  t.same(monster1Genome.slice(0,7), [122, 1, 0, 0, 179, 1, 0]);

  const monster2Payment = await buyMonster(bucksPayment2);
  const monster2Amount = await E(monsterIssuer).getAmountOf(monster2Payment);
  const [ monster2Serial ] = monster2Amount.extent;
  const monster2Genome = await E(privateAPI).genome(monster2Serial);
  t.same(monster2Genome.slice(0,7), [116, 13, 231, 246, 61, 197, 59]);

  const breedInvite = await E(publicAPI).makeBreedInvite();
  const breedProposal = harden({ give: { Fee: bucks5, P1: monster1Amount, P2: monster2Amount } });
  const breedKeywordRecord = harden({
    Fee: bucksPayment3,
    P1: monster1Payment,
    P2: monster2Payment,
  });
  const { payout: breedPayoutP } = await E(zoe).offer(breedInvite, breedProposal, breedKeywordRecord);
  const breedPayout = await breedPayoutP;
  const {
    Fee: feeP,
    Monster: breedMonsterPaymentP,
    P1: p1ReturnPaymentP,
    P2: p2ReturnPaymentP,
  } = breedPayout;

  const p1Amount = await E(monsterIssuer).getAmountOf(p1ReturnPaymentP);
  const [ p1Serial ] = p1Amount.extent;
  const p1Genome = await E(privateAPI).genome(p1Serial);
  t.same(p1Genome.slice(0,7), [ 122, 1, 0, 0, 179, 1, 0 ]);

  const p2Amount = await E(monsterIssuer).getAmountOf(p2ReturnPaymentP);
  const [ p2Serial ] = p2Amount.extent;
  const p2Genome = await E(privateAPI).genome(p2Serial);
  t.same(p2Genome.slice(0,7), [116, 13, 231, 246, 61, 197, 59]);

  const breedMonsterAmount = await E(monsterIssuer).getAmountOf(breedMonsterPaymentP);
  const [ breedMonsterSerial ] = breedMonsterAmount.extent;
  const breedMonsterGenome = await E(privateAPI).genome(breedMonsterSerial);
  t.same(breedMonsterGenome.slice(0,7), [120, 1, 97, 198, 187, 193, 32]);

One interesting thing is that, aside from the test assertions, this code consists almost 100% of const declarations. This isn't a bad thing, but it is a little unusual. It's almost like we're writing in one of those single-assignment languages.

But the main thing I wanted to point out here is the vast number of steps involving taking things out of other things to get more things to take even more things out of. And some of these things are promises, so there's that too.

A number of the Zoe operations return promises for records that resolve to collections of more promises. What's weird is that this makes for a potential promise pipeline whose potential must go unrealized because of the need to insert awaits so the records can be destructured to get at the next layer of promises. Interestingingly, eventual-get can do a single property extraction but we don't have a syntax for an eventual destructuring (though one could write a series of single-property eventual-gets to achieve the same result, but it would be less compact and involve some repetition).

I count 18 awaits in the above code excerpt, and I suspect many of them are unnecessary, at least in principle, but are forced on us by the API affordances. One question I'm left with is whether some of the Zoe method parameters that take non-promises could (or in fact, do) take promises instead.

For example, in the buyMonster function, we have

    const buyInvite = await E(publicAPI).makeBuyInvite();

then later we have

    const { payout: buyPayoutP } = await E(zoe).offer(buyInvite, buyProposal, buyKeywordRecord);
    const buyPayout = await buyPayoutP;
    const { Fee: feeP, Monster: buyMonsterPaymentP } = buyPayout;
    return buyMonsterPaymentP;

I observe in passing that the Fee portion of the buy payout is ignored here. In a production context it definitely shouldn't be, since it's how the contract returns any unspent portion of the purchase funds, but here it doesn't matter so I'm going to exploit that fact. So let's rewrite this as if it were promises all the way (heck, while we're at it, let's use tildot!):

    const buyInviteP = publicAPI~.makeBuyInvite();
    ...
    return zoe~.offer(buyInviteP, buyProposal, buyKeywordRecord)~.payout~.Monster;

Two changes (aside from tildot) make this work: (a) Zoe's offer method takes a promise in the invite parameter (and maybe it already allows that?) and (b) I'm not doing any destructuring, so I can extract the individual properties of interest from results with eventual-get.

But what if actually want to use that Fee result, in something like (harkening back to the original formulation):

    const { payout: buyPayoutP } = await E(zoe).offer(buyInvite, buyProposal, buyKeywordRecord);
    const buyPayout = await buyPayoutP;
    const { Fee: feeP, Monster: buyMonsterPaymentP } = buyPayout;
    const fee = await feeP.
    E(myWallet).deposit(fee);
    return buyMonsterPaymentP;

(where the wallet deposit is something I just made up for purposes of this example). If we go with the promises-all-the-way-down approach, we still need something like:

    const buyPayoutP = zoe~.offer(buyInviteP, buyProposal, buyKeywordRecord)~.payout;
    myWallet~.deposit(buyPayoutP~.Fee);
    return buyPayoutP~.Monster;

This is actually not too bad, but we've had to do two separate eventual-gets on buyPayoutP, which perhaps foregoes a chance to optimize with destructuring. Imagine being able to do something like:

    const ~{ Fee: feeP, Monster: buyMonsterPaymentP } = buyPayoutP;

which maybe lets us do the destructuring on the other end of the connection and only send back the properties that were asked for. This is a totally made up syntax, of course; I'm not sure what syntax would be good here, but I can imagine we could think up something less lame.

If we rewrite that large fragment above in this mode, we get something like this:

  const monsterIssuerP = publicAPI~.getMonsterIssuer();

  async function buyMonster(payment) {
    const buyInviteP = publicAPI~.makeBuyInvite();
    const buyProposal = harden({ give: { Fee: bucks5 } });
    const buyKeywordRecord = harden({ Fee: payment });
    const buyPayoutP = zoe~.offer(buyInvite, buyProposal, buyKeywordRecord)~.payout;
    const ~{ Fee: feeP, Monster: buyMonsterPaymentP } = buyPayoutP;
    return buyMonsterPaymentP;
  }

  const monster1PaymentP = buyMonster(bucksPayment1);
  const monster1AmountP = monsterIssuer~.getAmountOf(monster1Payment);
  const ~[ monster1SerialP ] = monster1AmountP~.extent;
  const monster1Genome = await privateAPI~.genome(monster1SerialP);
  t.same(monster1Genome.slice(0,7), [122, 1, 0, 0, 179, 1, 0]);

  const monster2PaymentP = buyMonster(bucksPayment2);
  const monster2AmountP = monsterIssuer~.getAmountOf(monster2Payment);
  const ~[ monster2SerialP ] = monster2AmountP~.extent;
  const monster2Genome = await privateAPI~.genome(monster2SerialP);
  t.same(monster2Genome.slice(0,7), [116, 13, 231, 246, 61, 197, 59]);

  const breedInviteP = publicAPI~.makeBreedInvite();
  const breedProposal = harden({ give: { Fee: bucks5, P1: monster1AmountP, P2: monster2AmountP } });
  const breedKeywordRecord = harden({
    Fee: bucksPayment3,
    P1: monster1PaymentP,
    P2: monster2PaymentP,
  });
  const breedPayoutP = zoe~.offer(breedInvite, breedProposal, breedKeywordRecord)~.payout;
  const ~{
    Fee: feeP,
    Monster: breedMonsterPaymentP,
    P1: p1ReturnPaymentP,
    P2: p2ReturnPaymentP,
  } = breedPayoutP;

  const ~[ p1Serial ] = monsterIssuer~.getAmountOf(p1ReturnPaymentP)~.extent;
  const p1Genome = await privateAPI~.genome(p1SerialP);
  t.same(p1Genome.slice(0,7), [ 122, 1, 0, 0, 179, 1, 0 ]);

  const ~[ p2Serial ] = monsterIssuer~.getAmountOf(p2ReturnPaymentP)~.extent;
  const p2Genome = await privateAPI~.genome(p2SerialP);
  t.same(p2Genome.slice(0,7), [116, 13, 231, 246, 61, 197, 59]);

  const ~[ breedMonsterSerialP ] = monsterIssuer~.getAmountOf(breedMonsterPaymentP)~.extent;;
  const breedMonsterGenome = await privateAPI~.genome(breedMonsterSerialP);
  t.same(breedMonsterGenome.slice(0,7), [120, 1, 97, 198, 187, 193, 32]);

This reduces the awaits from 18 to 5, and the 5 are the exact places where it's necessary to actually look at the bits.

erights commented 4 years ago
const ~{ Fee: feeP, Monster: buyMonsterPaymentP } = buyPayoutP;

try

const { Fee: feeP, Monster: buyMonsterPaymentP } = E.G(buyPayoutP);

This works now, but with an extra round trip as you say. However, once we do the work to pipeline eventual gets, this same expression will pipeline fine. So we should write this way assuming pipelined eventual gets.

In allowing even restricted awaits, I do worry people will write overly sequential code and miss these pipelining opportunities. When a then is necessary, a restricted await can be more pleasant. But both force round trips, destroying pipelining in ways we can't repair. Eventual gets currently destroy pipelining, but we will fix that.

FUDCo commented 4 years ago

try

const { Fee: feeP, Monster: buyMonsterPaymentP } = E.G(buyPayoutP);

How does this square with ~. syntax? Long term, JS isn't going to have the E.G(... thing.

erights commented 4 years ago

Good question. This is the one good use of E and E.G that currently has no syntactic equivalent. Your suggested syntax is possible but seems awkward. I'm inclined not to propose any such sugar to tc39 yet, while we gain experience using the unsugared E.G form.

FUDCo commented 4 years ago

Yah, the syntax I used there was just made up on the spot with very little thought invested, merely to have a placeholder for illustrative purposes; it was not intended as a serious proposal. But I think there's a grammatically ergonomic hole to fill here.

kriskowal commented 4 years ago

My feedback:

Design

I am still struggling with the Zoe API, but my understanding has improved. I generally agree with the design philosophy of building blocks before buildings. The sensation I have is that it is not done, and I need to think more to provide productive feedback.

Names

ERTP terminology feels solid to me. Mint, issuer, purse, amount, and payment are all good terms. I found extent odd and suspect that value would be less odd for fungible cases and forgivable for non-fungible cases.

Zoe terminology could use some polish.

I would like to encourage "invitation" instead of "invite" despite the brevity of the latter. Verbs lead the eye to functions and nouns to objects. I would also avoid abbreviation by truncation, like desc. It’s harder to memorize an API when you can’t be sure whether a word might or might not be truncated.

“Offer” doesn’t seem to be working for the most general case. I’ve heard “position” and “seat”. I like those.

I am actually in favor of keeping want and give as “keyword arguments”. I would find positional arguments confusing. For example, I’m never sure about the order of expected and actual to test assertions.

I like the new name “trade” and found “reallocate” awkward.

Deployment

The deployment system is cool. It is revelatory that building and submitting bundles or archives is public-facing.

As noted elsewhere, there is a missing step in the guide for Dapp Encouragement. Although there’s a call-out for running agoric deploy contract/deploy.js and api/deploy.js, we all seemed to miss ui/deploy.js, which is necessary to set up the wallet.

This is a temporary problem since we need to do something else for introducing purses for new mints and assigning pet names. It was concerning that, having failed to set up wallets for Assurance or Monsters, the application still worked but the user had no access to their purses.

It’s clear that this system needs to become part of a bigger system for production deployment that can hand-off references to clusters of application handlers.

Deployment is also oddly order and timing sensitive. I found that during development, it was better to restart and reset the ag-solo and then each deployment script in order. It might be valuable to invest in a developer experience more like the Parcel build-and-serve application (maybe even watch-build-and-serve).

Deploying an application probably ought to be more like a staged rocket, where each stage carries each of the subsequent stages, all the way through to the web server. Consider a system that submits a single application archive to the ag-solo that in turn contains the contract archive, api archive, and web application archive, then deploys each of these itself.

We probably ought to thread configuration like an endowment. Dropping configuration in a file for the next deployment script to pick up is pretty brittle, and I assume, temporary by design as fixing it would involve being able to deploy the UI too.

I would not miss the fs-watch web redeploy and would be excited if deployment were transactional on the whole application.

Getting Started Experience

We don’t currently release coherent cross-sections of the Agoric SDK to npm, so developers must use agoric install over yarn install in dapp repositories. We found that agoric install presents some limitations and some very strange developer experiences. My understanding is that we are deliberately favoring our own velocity and freedom to change the API.

Until we do pivot to publishing the SDK to npm, some of the developer speedbumps include:

My perception is that we should not have anything, including example applications, outside the Agoric SDK repository, until we can ensure some stability for external dependees.

Chris-Hibbert commented 4 years ago

I just wanted to highlight this suggestion:

Consider a system that submits a single application archive to the ag-solo that in turn contains the contract archive, api archive, and web application archive, then deploys each of these itself.

dtribble commented 4 years ago

Can invitations be used multiple times? If so, yeah, that sounds good. If not.. well, it's better than "hook", but implies (to me) multiple invocations, which isn't quite right.

Invites can only be used once, but "hooks" are typically used for multiple invites.

katelynsills commented 4 years ago

Thank you all for the feedback. This was incredibly helpful! There is still a lot to address and fix, but here are some of the changes that we've adopted in the new Zoe design as a direct result of these comments:

To highlight the changes using a snippet of @FUDCo's sample code:

  // users are expected to receive an invitation, not make one for themselves.
  function buyMonster(invitation, payment) {
    const proposal = harden({ give: { Fee: bucks5 } });
    const payments = harden({ Fee: payment });
    // a seat is returned when an offer is made
    const seat = E(zoe).offer(invitation, proposal, payments);
   // the promise for the payout of a particular keyword can be gotten directly
    return E(seat).getPayout('Monster');
  }
  // the creatorFacet is one of the things that can be returned in the call to `E(zoe).startContract(installation ...)`
  const invitation = E(creatorFacet).makeBuyInvitation();
  const monster1PaymentP = buyMonster(invitation, bucksPayment1);
  const monster1Amount = await E(monsterIssuer).getAmountOf(monster1PaymentP);
  const [ monster1Serial ] = monster1Amount.extent;
  // the creatorFacet can have any number of methods on it, without the creator of an instance having to make an offer to use the facet.
  const monster1Genome = await E(creatorFacet).genome(monster1Serial);
  t.same(monster1Genome.slice(0,7), [122, 1, 0, 0, 179, 1, 0]);
FUDCo commented 4 years ago

One small bit of feedback from updating the Agorables contract:

The change guide doc suggests a contract should start out with:

/**
 * @type {ContractStartFn}
 */
const start = zcf => {
...

The suggested pattern hides the parameter and return types under a typedef. Previously we had:

/**
 * The Agorables contract sells and breeds monsters for a fee.
 *
 * @typedef {import('@agoric/zoe').ContractFacet} ContractFacet
 * @param {ContractFacet} zcf
 */
function makeContract(zcf) {
...

The new type provides a more complete description of the function, but poorer documentation. I think it would be better to expand it, e.g.:

/**
 * The Agorables contract sells and breeds monsters for a fee.
 *
 * @param {ContractFacet} zcf
 *
 * @returns {ContractStartFnResult}
 */
function start(zcf) {
...

When everything is hidden under a typedef the only way to understand the function signature is to either view the source in an IDE and mouse over the type name or go hunting for the type in the types.js file (which, you'll note, does not get imported directly so one in addition has to hunt for the types file itself first). Since the source gets viewed in various non-IDE contexts (most importantly GitHub but also non-IDE editors), the meaning of the source text is more obscure than it needs to be. I think it would be clearer to be explicit

(I also think the "assign an arrow function to a variable" pattern is terrible, but in this case I have the option to write my function definition in the form of a function definition and type checking still works)

I'm also a little skeptical of the ContractStartFnResult type too, since it seems to be specialized to this function and therefor might also benefit from being expanded inline, but I understand this could get hairy and tradeoff in the other direction.

erights commented 4 years ago

(I also think the "assign an arrow function to a variable" pattern is terrible, but in this case I have the option to write my function definition in the form of a function definition and type checking still works)

As assignment would indeed be terrible. This is a const definition ;) . In all seriousness, we should discuss it. I have come to appreciate avoiding function functions.