refactor: use named fields in atomicRearrange #65
Conversation
contract/src/offer-up.contract.js
Outdated
| ); | ||
| const charge = { Price: tradePrice }; | ||
| atomicRearrange(zcf, [ | ||
| { fromSeat: buyerSeat, toSeat: proceeds, fromAmounts: charge }, |
There was a problem hiding this comment.
Don't bake the types into the property names ("bad Hungarian"). So perhaps
'{ from: buyerSeat, to: proceeds, amounts: charge, mappedTo: zot },'
where mappedTo: zot is the optional amountKeywordRecord to rearrange the amounts to.
There was a problem hiding this comment.
Just for the record, I find a mild amount of Hungarian notation better that none, and better than too much.
That said, I like the names @dtribble suggests here. mappedTo took awhile to appreciate, but it grew on me.
There was a problem hiding this comment.
Don't bake the types into the property names
Those names came from the existing type declaration. I suppose the existing type is a tuple, where the names don't show up at runtime. ok, done: 6df24ef
There was a problem hiding this comment.
Please restore the
harden([
First I'd like to understand why @dtribble thinks it's not called for.
I think I understand why @erights is asking to put it back:
our docs say:
It's important to harden() an object before exposing the object by returning it or passing it to some other function.
That's a friendly version of the more precise text from the Jessie readme:
in Jessie all objects made by literal expressions (object literals, array literals, the many forms of function literals) must be tamper-proofed with harden before they can be aliased or escape from their static context of origin.
There was a problem hiding this comment.
to me, harden is not called for because the arg is an explicit literal, so we cannot have any ability to mutate it, and cannot detect/care if the callee mutates it. If I had a lint rule, I would want that scenario to not be an issue. I'm generally in favor of APIs that harden their args so that the burden is not on the caller, but you guys get to figure that out :).
There was a problem hiding this comment.
Just for the record, I find a mild amount of Hungarian notation better that none, and better than too much.
That said, I like the names @dtribble suggests here.
mappedTotook awhile to appreciate, but it grew on me.
I also like a mild amount of it, and don't have any objection to "good Hungarian" where it's documenting the role of value, not it's type (e.g. "fooIndex" vs. "fooInt"). The confusion here is because some have it and some don't. With naming (e.g.., from/to) there's enough new orientation that it matters less. The name proceeds is very subtle for such a central thing in the contract.
There was a problem hiding this comment.
but you guys get to figure that out :).
I am trying to shift us from using objects as an intra-vat defensive boundary to use exo interfaces as a defensive boundary. The exo interfaces certainly are a good defensive boundary in many ways. But they're not yet good enough to complete the shift because of promises that can resolve later to things that violate the boundary. @michaelfig and I talked about a progression of promise-likes that would solve the whole promise problem for this purpose. AFAICT, vows are successfully going in this direction. M.await even gives us a safe way to handle arguments that remain promises rather than vows.
I look forward to having the time to get back to these problems. ;)
In the meantime, don't drop harden.
contract/src/offer-up.contract.js
Outdated
| ); | ||
| const charge = { Price: tradePrice }; | ||
| atomicRearrange(zcf, [ | ||
| { fromSeat: buyerSeat, toSeat: proceeds, fromAmounts: charge }, |
There was a problem hiding this comment.
Just for the record, I find a mild amount of Hungarian notation better that none, and better than too much.
That said, I like the names @dtribble suggests here. mappedTo took awhile to appreciate, but it grew on me.
contract/src/offer-up.contract.js
Outdated
| ); | ||
| const charge = { Price: tradePrice }; | ||
| atomicRearrange(zcf, [ | ||
| { fromSeat: buyerSeat, toSeat: proceeds, fromAmounts: charge }, |
| transferParts.map(part => | ||
| Array.isArray(part) | ||
| ? part | ||
| : [part.fromSeat, part.toSeat, part.fromAmounts, part.toAmounts], |
dckc
force-pushed
the
dc-transfer-named-parts
branch
from
2c085f5 to
6df24ef
Compare
|
Please excuse the force-push. This is stacked on #61 and some changes that belong there had leaked into this one. |
| * from?: ZCFSeat, | ||
| * to?: ZCFSeat, | ||
| * amounts?: AmountKeywordRecord, | ||
| * mappedTo?: KeywordKeywordRecord |
There was a problem hiding this comment.
I am surprised by this. When I saw @dtribble 's suggestion that the optional fourth field be named mappedTo, it had not occurred to me that it should be a KeywordKeywordRecord for mapping the entries in amounts. I was just thinking that it was an alternative to the name toAmounts.
On first blush, both are plausble. As an AmountKeywordRecord, the original meaning of toAmounts (whatever it is called) is more expressive, but perhaps not usefully so. Independent of naming, what are y'all's thoughts on which is the better API design?
Btw, when I said mappedTo grew on me, I only had in mind that it was just a renaming of the original fourth AmountKeywordRecord optional argument. But the possibility of having it be a KeywordKeywordRecord had not occurred to me. Given that this choice also makes sense, I also agree that the name mappedTo is only a good name if we choose the KeywordKeywordRecord meaning for it.
There was a problem hiding this comment.
Given that the amounts must match, the KeywordKeywordRecord has the virtue of not requiring redundant specification, but it also means you can't split up the amounts differently. I think cases where you need to do that are rare, and can be covered in fromOnly and toOnly.
| ]), | ||
| ); | ||
| const charge = { Price: tradePrice }; | ||
| atomicRearrange(zcf, [ |
There was a problem hiding this comment.
Sorry, already stated. (I thought I was commenting on another PR where I hadn't yet said this.)
@dtribble noted that the call to
atomicRearrange()was awkward.I'd like to be able to use named fields there. This provides and then uses a version that works that way.
cc @erights @Chris-Hibbert @mhofman