OfferTo as a Zoe Helper - ready for design review

[katelynsills]

In this PR, offerTo is written as a helper. It is incorporated into the loan's liquidation code and the OTC desk code with those contracts' tests passing. This code is in a good state for a cursory design review of the offerTo signature:

offerTo(
  zcf,
  invitation,
  keywordMapping = {},
  proposal,
  fromSeat,
  toSeat
);

Types:

/**
 * @callback OfferTo
 * @param {ContractFacet} zcf
 * @param {ERef<Invitation>} invitation
 * @param {KeywordKeywordRecord=} keywordMapping
 * @param {Proposal} proposal
 * @param {ZCFSeat} fromSeat
 * @param {ZCFSeat} toSeat
 * @returns {OfferToReturns}
 */

Where invitation is the invitation to contractB, proposal is the proposal to use with the invitation, and keywordMapping is a record of the keywords appropriate to contractA mapped to the keywords for contractB.

Note that when then offerTo implementation is moved into Zoe in another PR, we will be able to get rid of the zcf parameter:

 const { userSeatPromise: autoswapUserSeat, deposited } = zcf.offerTo(
   swapInvitation,
   keywordMapping, // {}
   proposal,
   fromSeat,
   lenderSeat,
 );

[Chris-Hibbert]

Note that when then offerTo implementation is moved into Zoe, we will be able to get rid of a few parameters since it will probably be a method on the fromSeat fromSeat.offerTo(invitation, proposal, fromAssets, toSeat, keywordMapping);

I'm surprised that it would make sense to give the responsibility to one zcfSeat to reallocate the results to another seat. I agree that that reduces the number of extraneous parameters, but it seems like an odd place for the behavior to go.

Just off the top of my head, but something like one of these

toSeat.transferFrom(fromSeat.transferVia(invitation, proposal, fromAssets), keywordMapping);

or

fromSeat.transferVia(invitation, proposal, fromAssets).transferTo(toSeat, keywordMapping);

[katelynsills]

Note that when then offerTo implementation is moved into Zoe, we will be able to get rid of a few parameters since it will probably be a method on the fromSeat fromSeat.offerTo(invitation, proposal, fromAssets, toSeat, keywordMapping);

I'm surprised that it would make sense to give the responsibility to one zcfSeat to reallocate the results to another seat. I agree that that reduces the number of extraneous parameters, but it seems like an odd place for the behavior to go.

Just off the top of my head, but something like one of these

toSeat.transferFrom(fromSeat.transferVia(invitation, proposal, fromAssets), keywordMapping);

or

fromSeat.transferVia(invitation, proposal, fromAssets).transferTo(toSeat, keywordMapping);

Hmm, it makes sense to me. The fromSeat is being instructed to use its allocated amounts to make an offer to another contract. This makes sense for two reasons: this call only works if it upholds offer safety for the fromSeat, which we already check on the zcfSeat: zcfSeat.isOfferSafe, and second, the deposit often goes back to the fromSeat. In other words, often the use case only involves one zcfSeat.

Your two proposals would fail as written because both the making of the offer and the depositing of the payouts need the keywordMapping, but you've only put it in one of the two sides.

[Chris-Hibbert]

This looks really good. I have some very local comments on docs and formatting.

[Chris-Hibbert]

shouldn't this have an @returns clause?

[katelynsills]

Ah good catch, thanks!

[katelynsills]

Hmm, I just tried this and if I say that the return value is {AmountKeywordRecord | PaymentPKeywordRecord | undefined }, it is not smart enough to know that the same kind will be returned. I don't know how to specify this. But with this return value, I get errors in offerTo saying that I can't pass something potentially undefined to the other methods that use the result of mapKeywords

[Chris-Hibbert]

Any reason not to use Promise.all([deposited, offerResult]).then(?

[katelynsills]

We have to ensure the deposit on the lenderSeat is done before the lenderSeat is exited.

[Chris-Hibbert]

My point was that in both cases, we wait for both promises to complete. The present code waits for them in order, but doesn't force the deposit to happen before the offer. The offerResult promise was created earlier, and its execution is unblocked.

The reason I suggested the change is because the consistent treatment of the two promises that we're waiting for with a single Promise.all() seems cleaner syntactically.

The only difference in execution that I can see is that if deposited fails, the present code wouldn't call closeWithFailure, while the combined version would. I presume this would actually be an improvement, and the reason it wasn't done initially was that a failure in the deposit was seen as unlikely.

[katelynsills]

Hmm, let me think about this.

[katelynsills]

Ah, ok, I think I figured out what is going on here. You're right that in the case that both promises resolve, we wait for deposited to resolve before the lenderSeat exits, so that case works. However, when the offerHandler throws, we try to exit the lenderSeat before it is deposited to. This is because Promise.all() rejects immediately if any of the promises reject individually, so what happens if we have Promise.all([deposited, offerResult]).then( is that we do not await deposited before trying to exit the lenderSeat in closeWithFailure. But, we do want to always deposit first, so that's not the right behavior.

We want to await deposited regardless of whether offerResultP rejects or not, so I think the original version is the better option.

[katelynsills]

Good questions. If deposited rejects, then we have profoundly misunderstood something and it's unclear how we should recover. This would mean that we could not deposit the payouts (refund or otherwise) to the lenderSeat. We do have access to the payouts through the autoswapUserSeat but it's not clear how we would deposit again in a way that would be successful given that our earlier attempt failed. The reason why offerResultP rejecting is handled better is that it assumes that deposited likely resolves due to a refund payout, and we can safely exit lenderSeat because it got the refund.

Likewise, if deposited never settles, something has gone horribly wrong, because 1) autoswap always exits the seat quickly and 2) even in the case of error in autoswap, we should still get a payout that we can deposit safely. Without having deposited a refund successfully it is unclear how we should recover. For instance, we don't want to exit seats or shutdown the contract if only the contract has access to payments.

[katelynsills]

Let me know if any of these conversations should hold up the PR. Chris has already approved and I think it's ready to merge once the CI passes

[erights]

If our definition of issuer misbehavior includes rejection or never settling under these conditions, then the behavior you describe is consistent with our threat model for payout liveness. For any given seat, a misbehaving issuer among the issuers that this seat explicitly mentions in its offer, that issuer can block payout liveness for the seat as a whole. This is just another instance of that same allowed failure mode. The security claim we need to make for payout liveness in the face of issuer misbehavior is only

A misbehaving issuer cannot block payout liveness for seats that do not mention that issuer as an issuer in their offer.

Btw, we need to gather such precise statements of the security claims we are making, both for what they include and what they exclude.

[erights]

Let me know if any of these conversations should hold up the PR. Chris has already approved and I think it's ready to merge once the CI passes

I still see no reason for this to block on me. Please proceed.

[erights]

Btw, we need to gather such precise statements of the security claims we are making, both for what they include and what they exclude.

For this claim, already done though it should be refined.

https://github.com/Agoric/agoric-sdk/wiki/Limited-Risk-from-Bad-Issuers