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

[Zoe] Add roles #532

Closed
wants to merge 25 commits into from
Closed

[Zoe] Add roles #532

wants to merge 25 commits into from

Conversation

katelynsills
Copy link
Contributor

@katelynsills katelynsills commented Feb 12, 2020

This is a WIP PR to add "roles" to Zoe.
Closes #97
Closes #392
Closes #469
Closes #564
Closes #677

Tests that don't yet pass:

Kate responsibility:

  • zoe - autoswap - valid inputs - with SES
  • autoSwap with valid offers
  • autoSwap - test fee

Chris responsibility:

  • zoe - simpleExchange - valid inputs - with SES
  • zoe - simpleExchange - state Update - with SES
  • myFirstDapp with valid offers
  • myFirstDapp with multiple sell offers
  • myFirstDapp showPayoutRules
  • simpleExchange with valid offers
  • simpleExchange with multiple sell offers
  • simpleExchange showPayoutRules

Still to do:

  • fix autoswap
  • fix simpleExchange
  • fix myFirstDapp
  • Remove old code and helpers
  • Assess whether old payoutRules structure is still helpful at all
  • Look over and clean up
  • Close this Draft and reopen a new PR for the Zoe release branch

@katelynsills katelynsills added the Zoe package: Zoe label Feb 12, 2020
@katelynsills katelynsills added this to the Zoe 0.3.0 milestone Feb 12, 2020
@katelynsills katelynsills self-assigned this Feb 12, 2020
@katelynsills
Copy link
Contributor Author

katelynsills commented Feb 27, 2020

This PR is ready for an initial design review. I would recommend starting with the coveredCall test (test.only: zoe - coveredCall) and looking at the changes from the user's perspective, then looking at Zoe.js and state.js.

I'll continue updating the tests while the review is happening in parallel.

@katelynsills katelynsills changed the base branch from master to no-config-add-brand February 27, 2020 22:23
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 will look at zoe.js tomorrow. This is as far as I got today.

packages/zoe/src/zoe.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.

Review so far. I like where this is going --- the changes look much better than the original. This is a much better API.

I'm puzzled by enough things that I'll wait for some responses before resuming the review.

packages/zoe/src/cleanOfferRules.js Outdated Show resolved Hide resolved
packages/zoe/src/cleanOfferRules.js Outdated Show resolved Hide resolved
packages/zoe/src/cleanOfferRules.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/coveredCall.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/helpers/userFlow.js Outdated Show resolved Hide resolved
packages/zoe/src/state.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/coveredCall.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/helpers/userFlow.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/cleanOfferRules.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/helpers/userFlow.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/zoe.js Show resolved Hide resolved
packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
packages/zoe/src/state.js Show resolved Hide resolved
packages/zoe/src/state.js Outdated Show resolved Hide resolved
@katelynsills katelynsills force-pushed the no-config-add-brand branch from a73f46b to 6b39e84 Compare March 6, 2020 23:09
@katelynsills katelynsills changed the base branch from no-config-add-brand to master March 7, 2020 00:27
@katelynsills katelynsills linked an issue Mar 10, 2020 that may be closed by this pull request
@katelynsills
Copy link
Contributor Author

@Chris-Hibbert I added TODOs to the PR description. I assigned you to the SimpleExchange and MyFirstDapp changes. Let me know if that works!

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.

Yep. I'll work on those. I've pulled down your recent changes to the contracts and will continue from there.

@katelynsills katelynsills linked an issue Mar 12, 2020 that may be closed by this pull request
@Chris-Hibbert
Copy link
Contributor

checkIfOfferRules looks helpful. It's a little surprising that it didn't reduce the amount of code, but I think it does clarify the intent. I'll look into using it for simpleExchange.

@katelynsills
Copy link
Contributor Author

Closed in favor of #685

@katelynsills katelynsills deleted the 469-zoe-roles-2 branch March 25, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment