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.

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

Compare Swap and Trade Helpers and make more consistent #1667

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

katelynsills
Copy link
Contributor

Trade now checks both seats to see if either have exited. Trade does not kickOut any seat, but throws errors instead, which will result in the seat in the offerHandler being kickedOut naturally if not handled.

Swap now kicks out both seats if any error is thrown in trade. This is because our usage changed such that we often call zcf.shutdown() after a swap, so there is no good reason to treat the seats asymmetrically.

The distinctions between swap and trade still apply:

Screen Shot 2020-09-01 at 6 18 18 PM

Closes #1531

@katelynsills katelynsills added the Zoe package: Zoe label Sep 2, 2020
@katelynsills katelynsills self-assigned this Sep 2, 2020
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

looks good! One outdated comment.

packages/zoe/src/contractSupport/zoeHelpers.js Outdated Show resolved Hide resolved
keepHandleInactiveMsg = 'prior offer is unavailable',
leftSeat,
rightSeat,
leftHasExitedMsg = 'the left seat in swap() has exited',
Copy link
Member

Choose a reason for hiding this comment

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

Given async, let's add several elements to the error about e.g., what brands are being traded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, we can't currently display brands. What kind of information would you like to see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside, all of the information console logged within trade applies to swap too because swap uses trade.

Base automatically changed from 1662-kick-out to master September 2, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we consolidate trade and swap into one helper?
3 participants