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

Reallocate api patch #519

Merged
merged 13 commits into from
Jun 8, 2021
Merged

Reallocate api patch #519

merged 13 commits into from
Jun 8, 2021

Conversation

tyg
Copy link
Contributor

@tyg tyg commented Jun 7, 2021

Updated documentation to reflect Reallocate API changes and enhance coverage of ZCFSeats.

@tyg tyg requested review from katelynsills and Chris-Hibbert June 7, 2021 18:37
@tyg tyg self-assigned this Jun 7, 2021
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
Note that ZoeHelpers [`trade()`](./zoe-helpers.md#trade-zcf-left-right-lefthasexitedmsg-righthasexitedmsg)
and [`swap()`](./zoe-helpers.md#swap-zcf-leftseat-rightseat-lefthasexitedmsg-righthasexitedmsg) might
be easier to use for simple cases.
### `ZCFSeat.stage(newAllocation)`**DEPRECATED 22-06-01**
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's going to be gone, not deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. We should probably distinguish between methods that exist and are deprecated, and methods that are gone. I think at this point, we should remove anything from documentation that is removed in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although at some point, we do want to start leaving indicators in the docs for a while when we drop something, since someone will probably have been using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, agreed. I think we should start that after we finish this string of known breaking changes for Zoe and ERTP.

main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
Comment on lines 432 to 440
The reallocation is partial, only applying to `seats` associated
with the `seatStagings`. By induction, if rights conservation and
offer safety hold before, they hold after a safe reallocation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to say something new about offer safety. The existing versions verify offer safety as each staging is created, but the new approach is unable to do that, so offer safety is checked on reallocate, and can cause the transaction to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to say anything new about offer safety. We just need to remove anything that says "offer safety is checked at staging". Checking offer safety at the reallocation step is the model we had originally, and I don't think any contracts will have problems moving back to that model (nor will they notice).

main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
@katelynsills
Copy link
Contributor

Commenting here because this file wasn't yet included in this PR, but we'll need to remove the "SeatStagings" entry on line 429 of glossary/README.md

Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

I think we're pretty close on the reallocate API changes, but let's move working_with_ZCFSeats.md to a separate PR, as I think that will take a lot longer than our timeline for the reallocate change landing. I only reviewed to line 84 or so of working_with_ZCFSeats.md.

main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Show resolved Hide resolved
Comment on lines 275 to 276
- Adds the `amountKeywordRecord` argument to the `ZCFseat`'s staged allocation and returns the
resulting new staged allocation value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Adds the `amountKeywordRecord` argument to the `ZCFseat`'s staged allocation and returns the
resulting new staged allocation value.
- Adds the `amountKeywordRecord` argument to the `ZCFseat`'s staged allocation and returns the
same `amountKeywordRecord` so it can be reused in another call.

incrementBy returns the same exact amountKeywordRecord that was passed to it, which is why incrementBy(decrementBy()) works as a pattern of use. Maybe we should explain this pattern of use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/guide/working_with_ZCFSeats.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
main/zoe/api/zoe-contract-facet.md Outdated Show resolved Hide resolved
Note that ZoeHelpers [`trade()`](./zoe-helpers.md#trade-zcf-left-right-lefthasexitedmsg-righthasexitedmsg)
and [`swap()`](./zoe-helpers.md#swap-zcf-leftseat-rightseat-lefthasexitedmsg-righthasexitedmsg) might
be easier to use for simple cases.
### `ZCFSeat.stage(newAllocation)`**DEPRECATED 22-06-01**
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, agreed. I think we should start that after we finish this string of known breaking changes for Zoe and ERTP.

Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few small changes and the removal of main/zoe/guide/working_with_ZCFSeats.md that we talked about. Let's plan on merging this tomorrow in coordination with the agoric-sdk PR once that is approved.

@katelynsills
Copy link
Contributor

We can merge this, as Agoric/agoric-sdk#3184 is pretty much settled apart from an optional final review.

@katelynsills katelynsills merged commit bcdf0da into main Jun 8, 2021
@katelynsills katelynsills deleted the reallocate-API-patch branch June 8, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants