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

fix(exo): remove receiveRevoker #1964

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 14, 2024

closes: #XXXX
refs: #1666 Agoric/agoric-sdk#2070 Agoric/agoric-sdk#8020 Agoric/agoric-sdk#8745 Agoric/agoric-sdk#8749

Description

Reverse #1666's addition of receiveRevoker to the meta-operations of exos.

#1666 defined these for all exos and implemented them for heap exos, under the assumption that they would eventually be implemented in agoric-sdk for virtual and durable exos as well. Doing so turned out to be harder than expected Agoric/agoric-sdk#2070 , leading the attempt to do so Agoric/agoric-sdk#8020 to instead resort to a shim emulation that is neither scalable nor durable, and so unfit for production.

The main motivation for revocables is as a lower layer abstraction supporting ownables at a higher layer. But the receiveRevoker approach at a low layer led to the Agoric/agoric-sdk#8517 approach to ownables at a higher layer, which was a failed experiment.

Rather, Agoric/agoric-sdk#8745 tries a caretaker approach to revocables at a low level, leading to a pleasant API for ownables at a high level. @dckc's comment at Agoric/agoric-sdk#8745 (comment) suggested refactoring these to depend on zones rather than baggage. Once done, then the caretaker-like revocables become independent of heap vs virtual vs durable, and could be migrated to endo as an alternate form of revocation. But this will depend on first moving baseZone and store to endo, whose first step is Agoric/agoric-sdk#8749

Originally: This change is flagged as fix(exo)!: because it is a compat break. But we are unaware of any uses in production, so it may be a compat break only in theory.

Update: From reviewer suggestions below, I changed this to fix(exo):.

Note that Agoric/agoric-sdk#8021 is the only other PR I know that depends on receiveRevoker, and it is Draft.

Once receiveRevoker is gone, the other similar meta-operations are

  • receiveAmplifier, which is already implemented for heap exos.
  • receiveInstanceTester in feat(exo): instance testing #1925 , which defines it and implements it for heap exos.

Unlike receiveRevoker, I expect both of these will be easy to implement in agoric-sdk, although this has not yet been tried.

Security Considerations

Fewer privileged meta operations reduced our attack surface. But since this API was already designed to facilitate auditing, that's a minor worry.

Much more important is that removing this from the exo definition means that we no longer need to implement it for virtual and durable exos, enabling us to avoid or postpone Agoric/agoric-sdk#2070 until we need it for other reasons (like Agoric/agoric-sdk#8021 ). Solving Agoric/agoric-sdk#2070 would have been major surgery on a liveslots, which is a security critical component of our system. Better to postpone that risk if we can.

Scaling Considerations

Solving Agoric/agoric-sdk#2070 , enabling uses like Agoric/agoric-sdk#8021 would be a major help to scalability. Postponing it postpones this scalability benefit. But this PR itself doesn't cause it to be postponed further. It merely copes with its likely delay.

Documentation Considerations

Fewer exo meta-operations to document.

Testing Considerations

Fewer exo meta-operations to test. In particular, testing the outcome of fixing Agoric/agoric-sdk#2070 would have been interesting ;) .

Upgrade Considerations

For this PR itself, none. For Agoric/agoric-sdk#2070 , it would be good to be able to upgrade a durable non-revocable exo class to a revocable one, and perhaps even vice versa. The receiveRevoker API was designed to be consistent with that. But the real question is the representation of potentially revocable durable objects.

@erights erights self-assigned this Jan 14, 2024
@erights erights force-pushed the markm-remove-receiveRevoker branch from 90f7bba to 8e163e0 Compare January 14, 2024 03:54
@erights erights requested a review from mhofman January 14, 2024 03:58
@mhofman
Copy link
Contributor

mhofman commented Jan 14, 2024

This change is flagged as fix(exo)!: because it is a compat break. But we are unaware of any uses in production, so it may be a compat break only in theory.

I'm wondering if we need a way to flag some new additions as experimental, allowing us to have breaking changes without semver major bumps. I think this is another one of these case which doesn't warrant triggering a major version.

@erights erights changed the title fix(exo)!: remove receiveRevoker fix(exo): remove receiveRevoker Jan 14, 2024
@erights erights force-pushed the markm-remove-receiveRevoker branch from 8e163e0 to 0efad6f Compare January 14, 2024 21:15
@erights
Copy link
Contributor Author

erights commented Jan 14, 2024

This change is flagged as fix(exo)!: because it is a compat break. But we are unaware of any uses in production, so it may be a compat break only in theory.

I'm wondering if we need a way to flag some new additions as experimental, allowing us to have breaking changes without semver major bumps.

Sounds plausible!

I think this is another one of these case which doesn't warrant triggering a major version.

Changed to fix(exo):. Done, thanks.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Looks legit to me.

@erights erights force-pushed the markm-remove-receiveRevoker branch from e771700 to 3a39841 Compare January 16, 2024 19:00
@erights erights enabled auto-merge (squash) January 16, 2024 19:02
@erights erights merged commit d848754 into master Jan 16, 2024
14 checks passed
@erights erights deleted the markm-remove-receiveRevoker branch January 16, 2024 19:07
erights added a commit that referenced this pull request Jan 16, 2024
erights added a commit that referenced this pull request Jan 16, 2024
erights added a commit that referenced this pull request Jan 17, 2024
erights added a commit that referenced this pull request Jan 18, 2024
erights added a commit that referenced this pull request Jan 19, 2024
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