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

Add escrow payment and allocate to recipient ContractSupport Helper #993

Merged
merged 6 commits into from
May 1, 2020

Conversation

katelynsills
Copy link
Contributor

@katelynsills katelynsills commented Apr 23, 2020

Closes #992
Closes #1010
Closes #1012
Closes #1013

@katelynsills katelynsills added the Zoe package: Zoe label Apr 23, 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.

just a correction on an internal comment. otherwise, LGTM

packages/zoe/src/contracts/mintPayments.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/mintPayments.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/mintPayments.js Outdated Show resolved Hide resolved
packages/zoe/src/contractSupport/zoeHelpers.js Outdated Show resolved Hide resolved
Copy link
Member

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

Please revert that last change. It isn't quite what I have in mind, and the previous state is a better starting point. I also am not certain the refactoring I have in mind works.

Since the code as currently written is correct and follows the style of other approved code, I'm gonna say LGTM without this last commit. Approved to merge.

Then, when we have the time, let's look at refactorings of this pattern.

@DavidBruant
Copy link
Contributor

If i recall correctly, the opera ticket contract and autoswap were users of the previous pattern and should benefit from the new helper
I think it'd make sense to change them so the pattern of self-invitation disappears from contract codes

I don't have an opinion on whether this work should be done as part of this PR or a folow-up one

@katelynsills
Copy link
Contributor Author

If i recall correctly, the opera ticket contract and autoswap were users of the previous pattern and should benefit from the new helper
I think it'd make sense to change them so the pattern of self-invitation disappears from contract codes

I don't have an opinion on whether this work should be done as part of this PR or a folow-up one

Good idea. I like including their changes in this PR.

@katelynsills
Copy link
Contributor Author

Meta note: since this is adding a helper that hackathon participants probably need ASAP, and is not changing the Zoe API, I'm going to keep the base on this as master

@katelynsills katelynsills force-pushed the 992-escrow-and-allocate-helper-2 branch 2 times, most recently from 61bb1ef to 09830a8 Compare April 27, 2020 20:25
@katelynsills
Copy link
Contributor Author

katelynsills commented Apr 28, 2020

@erights, I had to make two bug fixes in the latest commit:

Offer Safety Check with Sparse Keywords

#1012
If not all 'give' keywords are in the allocation, that should be ok, but refundOk should be false. Right now it throws:

Screen Shot 2020-04-27 at 5 07 08 PM

updateAmounts with Sparse Keywords

#1013
We shouldn't be overwriting the allocations if the allocations might be partial, using sparse keywords
Screen Shot 2020-04-27 at 5 07 21 PM

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.

I'm fine with this change.

Copy link
Member

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

LGTM

@katelynsills katelynsills force-pushed the 992-escrow-and-allocate-helper-2 branch from 235a8f5 to 41b6c9d Compare May 1, 2020 18:15
@katelynsills katelynsills merged commit 60e384a into master May 1, 2020
@katelynsills katelynsills deleted the 992-escrow-and-allocate-helper-2 branch May 1, 2020 18:35
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
4 participants