-
Notifications
You must be signed in to change notification settings - Fork 218
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
[Zoe] Add roles #532
Conversation
b9b2217
to
23995fd
Compare
This PR is ready for an initial design review. I would recommend starting with the coveredCall test (test.only: I'll continue updating the tests while the review is happening in parallel. |
There was a problem hiding this 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.
2b21828
to
120438c
Compare
There was a problem hiding this 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.
a73f46b
to
6b39e84
Compare
8bd531e
to
5f24999
Compare
5f24999
to
2af9b6d
Compare
…hat should remain passing
@Chris-Hibbert I added TODOs to the PR description. I assigned you to the SimpleExchange and MyFirstDapp changes. Let me know if that works! |
There was a problem hiding this 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.
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. |
fix: Update SimpleExchange & myFirstDapp for roles
Closed in favor of #685 |
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:
Chris responsibility:
Still to do: