refactor ZoeSeat to drop cyclic structure that blocked GC #8682
Conversation
Chris-Hibbert
added
Zoe
Chris-Hibbert
force-pushed
the
8672-acyclicZoeSeatKit
branch
2 times, most recently
from
59596b6 to
0fe0d3c
Compare
There was a problem hiding this comment.
Do the existing tests cover the following case, or does this PR need an explicit test for it?
- Upgrading a zoe that already has some old style seats.
Even if that case is covered, would be to verify that the old seats differ from the new seats in the way you expect.
It's mentioned under Testing Considerations, above, as missing.
I presume that manual inspection in a debugging environment would suffice, as I don't know how to add this to a regression test. |
|
I presume that manual inspection in a debugging environment would suffice, as I don't know how to add this to a regression test.
Yes
|
| const OriginalZoeSeatIKit = harden({ | ||
| zoeSeatAdmin: M.interface('ZoeSeatAdmin', { | ||
| replaceAllocation: M.call(AmountKeywordRecordShape).returns(), | ||
| exit: M.call(M.any()).returns(), | ||
| fail: M.call(M.any()).returns(), | ||
| resolveExitAndResult: M.call({ | ||
| offerResultPromise: M.promise(), | ||
| exitObj: ExitObjectShape, | ||
| }).returns(), | ||
| getExitSubscriber: M.call().returns(SubscriberShape), | ||
| // The return promise is empty, but doExit relies on settlement as a signal | ||
| // that the payouts have settled. The exit publisher is notified after that. | ||
| finalPayouts: M.call(M.eref(PaymentPKeywordRecordShape)).returns( | ||
| M.promise(), | ||
| ), | ||
| }), | ||
| userSeat: M.interface('UserSeat', { | ||
| getProposal: M.call().returns(M.promise()), | ||
| getPayouts: M.call().returns(M.promise()), | ||
| getPayout: M.call(KeywordShape).returns(M.promise()), | ||
| getOfferResult: M.call().returns(M.promise()), | ||
| hasExited: M.call().returns(M.promise()), | ||
| tryExit: M.call().returns(M.promise()), | ||
| numWantsSatisfied: M.call().returns(M.promise()), | ||
| getFinalAllocation: M.call().returns(M.promise()), | ||
| getExitSubscriber: M.call().returns(M.any()), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
These seem identical with those in zoeSeat.js . Please de duplicate. If neither file should import the other, consider moving to a shared typeGuards.js . The commonality suggests that the interface is supposed to be the same.
Chris-Hibbert
force-pushed
the
8672-acyclicZoeSeatKit
branch
from
cdc16b8 to
006df25
Compare
|
Noting that #8517 provides a similar form of indirection in a more reusable manner, but bundled with ownability, which is not yet relevant. Still might be a good source to borrow from. |
Chris-Hibbert
force-pushed
the
8672-acyclicZoeSeatKit
branch
2 times, most recently
from
b2083b6 to
a2a9c2a
Compare
|
Note that I already approved. Once you're satisfied that #9009 shouldn't block this PR, feel free to merge. |
in zcfSeat.js: zcfSeatToSeatHandle.delete(self) in exit.js: state.zccfSeat = undefined
Chris-Hibbert
force-pushed
the
8672-acyclicZoeSeatKit
branch
from
a2a9c2a to
5d5722f
Compare
closes: #8672
Description
Refactor ZoeSeat to be composed of two separate Exos, so that dropping one can allow the cyclic structure described in #8672 to be GC'd.
Security Considerations
N/A
Scaling Considerations
Vast improvement in reducing retained garbage.
Documentation Considerations
Invisible to users.
Testing Considerations
Needs further testing in the upgrade environment to demonstrate
All existing tests in packages/zoe pass, with two tests requiring minor changes to error messages.
Upgrade Considerations
See above.