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.

Sign up for GitHub

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

Jump to bottom

Have proposal wants be amount patterns rather than just amounts #1905

Closed
wants to merge 1 commit into from
Closed

Have proposal wants be amount patterns rather than just amounts #1905

wants to merge 1 commit into from

Conversation

erights
Copy link
Member

@erights erights commented Oct 23, 2020 •
edited
Loading

WIP Spike Draft not ready for review

@erights erights self-assigned this Oct 23, 2020
@katelynsills
Copy link
Contributor

I might be missing your intention, but I was thinking that we don't change anything in ERTP but rather just change this line of Zoe:

agoric-sdk/packages/zoe/src/contractFacet/offerSafety.js

Line 21 in 559c5ed

return amountMath.isGTE(allocationAmount, requiredAmount);

@katelynsills
Copy link
Contributor

After talking it over, I agree the additive changes to amountMath are the right thing! Also, here's the places in our current tests where having more flexible wants would be helpful:

agoric-sdk/packages/zoe/test/unitTests/contracts/test-coveredCall.js

Line 494 in 559c5ed

want: { Asset: optionAmount },

agoric-sdk/packages/zoe/test/unitTests/contracts/test-coveredCall.js

Line 756 in 559c5ed

want: daveOptionValue.underlyingAssets,

agoric-sdk/packages/zoe/test/unitTests/contracts/test-sellTickets.js

Line 234 in 559c5ed

want: { Items: ticket1Amount },

agoric-sdk/packages/zoe/test/unitTests/contracts/test-sellTickets.js

Line 463 in 559c5ed

want: { Items: ticket2and3Amount },

@erights
Copy link
Member Author

erights commented Oct 24, 2020

Not really ready for review. But asking @katelynsills for help with problem discovered in swap and swapExact, where they're using the .want as gains in their calls to trade. trade assumes gains is an amount, not an amountPattern.

@erights erights changed the title WIP Have proposal wants be amount patterns rather than just amounts WIP Spike: Have proposal wants be amount patterns rather than just amounts Oct 26, 2020
This was referenced Oct 26, 2020
Open
Open
Closed
Closed
Closed
Closed
@erights
Copy link
Member Author

erights commented Oct 26, 2020 •
edited
Loading

We met and decided to shelve this #1905 spike because it is not required for hackathon. For those interested in looking at it anyway, the next step to make it actually work is to fix calcNewAllocations in zoeHelpers.js to work when toGains and fromLosses are AmountPatternKeywordRecords rather than AmountKeywordRecords. I don't actually know that this is hard; merely that I couldn't figure it out yet.

@erights erights mentioned this pull request Nov 2, 2020
Closed
erights added a commit that referenced this pull request Nov 3, 2020
@erights
fix: initial patching from #1905
c459233
@erights erights mentioned this pull request Nov 3, 2020
Merged
erights added a commit that referenced this pull request Nov 3, 2020
@erights
Preserve Marshal typing improvements from #1905 (#1965)
4d8cab8 
* fix: Add types to the marshal and same-structure packages
@katelynsills katelynsills removed their request for review November 19, 2020 16:38
This was referenced Dec 31, 2020
Closed
Closed
erights
erights commented Dec 31, 2020
View reviewed changes
Copy link
Member Author

@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.

@katelynsills let's discuss how it compares. Note that much of the bulk is changes to marshal that have already been migrated to master.

packages/ERTP/src/amountMath.js Outdated Show resolved Hide resolved
packages/ERTP/src/amountMath.js Outdated Show resolved Hide resolved
packages/same-structure/src/match.js Outdated Show resolved Hide resolved
packages/same-structure/src/match.js Outdated Show resolved Hide resolved
@erights erights force-pushed the amount-patterns branch 2 times, most recently from 19e184e to d30cc35 Compare January 1, 2021 01:08
@erights
Copy link
Member Author

erights commented Jan 1, 2021

rebased. All the redundancy with modern marshal disappeared.

@erights erights mentioned this pull request Jan 1, 2021
Merged
@erights
Copy link
Member Author

erights commented Jan 2, 2021

The findFromOtherSeat from #2156 transliterated into the amountPattern language at #2159 (a variant on this PR) WORKS!

The transliteration also revealed a bug in the original, repaired by in the new one by an isGround test. This prevents us from mistaking a pattern for a literal representation of assets.

Attn @katelynsills

@erights
Copy link
Member Author

erights commented Jan 2, 2021

No longer any reason to look at #2159 . All the relevant changes are now back in this PR

@erights erights changed the title WIP Spike: Have proposal wants be amount patterns rather than just amounts Have proposal wants be amount patterns rather than just amounts Jan 2, 2021
@erights erights marked this pull request as ready for review January 2, 2021 08:22
@erights erights marked this pull request as draft January 2, 2021 08:24
@erights
Copy link
Member Author

erights commented Jan 2, 2021

@katelynsills I assigned it to you because it is now a candidate to replace or subsume #2156 . But it is not yet a candidate to merge into master. My preference is that you take this over from here. It does seem to be in a good working state.

@katelynsills
Copy link
Contributor

Thank you @erights!

@erights erights mentioned this pull request Jan 4, 2021
Closed
@erights erights marked this pull request as ready for review January 11, 2021 05:06
@erights erights marked this pull request as draft January 11, 2021 05:07
@erights erights force-pushed the amount-patterns branch 2 times, most recently from 9f67079 to 5745944 Compare January 11, 2021 05:14
katelynsills
katelynsills reviewed Jan 14, 2021
View reviewed changes
packages/zoe/src/contracts/barterExchange.js
@@ -13,7 +13,7 @@ import { trade, satisfies } from '../contractSupport';
* https://agoric.com/documentation/zoe/guide/contracts/barter-exchange.html
*
* The Barter Exchange only accepts offers that look like
* { give: { In: amount }, want: { Out: amount} }
* { give: { In: amount }, want: { Out: amount} } // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @erights , what are these TODOs for?

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 remind me (now you ;) ) to look for all uses, to make sure those are compat with this being an AmountPattern rather than an Amount. Like one we saw in a recent PR ;)

@katelynsills katelynsills mentioned this pull request Jan 21, 2021
Open
13 tasks
@katelynsills katelynsills added Zoe package: Zoe Beta labels Feb 9, 2021
@katelynsills katelynsills added this to the Beta Launch milestone Feb 9, 2021
@erights
Copy link
Member Author

erights commented Feb 18, 2021 •
edited
Loading

See #2230

@katelynsills , do you plan to work in the PR or start a new one with this for reference? If the latter, should we close this one?

@katelynsills
Copy link
Contributor

This one is just reference. We can close as long as it keeps the branch around

@erights erights closed this Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katelynsills katelynsills katelynsills left review comments

@Chris-Hibbert Chris-Hibbert Awaiting requested review from Chris-Hibbert

@dtribble dtribble Awaiting requested review from dtribble

Assignees

@erights erights

@katelynsills katelynsills

Labels
Zoe package: Zoe
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erights @katelynsills