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: virtualize vPool as a facet #4707

Closed
wants to merge 2 commits into from
Closed

Conversation

erights
Copy link
Member

@erights erights commented Mar 1, 2022

Virtualize vPool as a facet

Staged on #4706

Compare with #4630 , which never worked

@erights erights self-assigned this Mar 1, 2022
@erights erights changed the base branch from master to markm-multi-facet-pool March 1, 2022 07:05
Comment on lines 19 to 26
export const makeSinglePool = (
zcf,
pool,
getProtocolFeeBP,
getPoolFeeBP,
feeSeat,
addLiquidityActual,
) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can you get rid of singlePool, but not doublePool? They wrap pool.js in similar ways.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a cycle between pool and singlePool (aka vPool). Creating a pool creates a vPool, and they point at each other. Following dependencies starting from pool.js I didn't encounter doublePool and so have not started thinking about how to virtualize it. What would you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure.

doublePool and singlePool are polymorphic wrappers of pool. Each pool has a single long-lived singlePool. doublePools connect two pools, and are created on demand.

I don't think it works to move singlePool's methods into pool`.

I don't think it's necessary for singlePool to be part of the persistent state. It would be fine for the singlePool to be created when the pool is created or revived.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, can anyone outside the vat be holding a remote reference to a singlePool? If so, are singlePools potentially high cardinality?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike vaults, clients don't hold onto any state of the AMM between transactions. All requests go through the public (or creator) interface for every request.

Copy link
Member Author

@erights erights Mar 22, 2022

Choose a reason for hiding this comment

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

Excellent! Then yes, I can and should simplify this. Thanks.

packages/run-protocol/src/vpool-xyk-amm/pool.js Outdated Show resolved Hide resolved
Comment on lines 19 to 26
export const makeSinglePool = (
zcf,
pool,
getProtocolFeeBP,
getPoolFeeBP,
feeSeat,
addLiquidityActual,
) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure.

doublePool and singlePool are polymorphic wrappers of pool. Each pool has a single long-lived singlePool. doublePools connect two pools, and are created on demand.

I don't think it works to move singlePool's methods into pool`.

I don't think it's necessary for singlePool to be part of the persistent state. It would be fine for the singlePool to be created when the pool is created or revived.

@erights erights force-pushed the markm-multi-facet-pool branch 4 times, most recently from 1592e43 to 2cfe4a3 Compare March 21, 2022 22:16
@erights erights force-pushed the markm-virtualize-vpool branch from 55b8df3 to aa07f79 Compare March 21, 2022 22:58
@erights erights mentioned this pull request Mar 21, 2022
@erights erights changed the base branch from markm-multi-facet-pool to markm-virtualize-ERTP March 21, 2022 23:02
@erights erights requested a review from Chris-Hibbert March 22, 2022 00:23
@erights erights force-pushed the markm-virtualize-ERTP branch 2 times, most recently from a4fb604 to a68c837 Compare March 22, 2022 19:53
Base automatically changed from markm-virtualize-ERTP to master March 22, 2022 20:47
@warner
Copy link
Member

warner commented Mar 23, 2022

@erights could you rebase this and force-push a new version? It looks like this branch includes slightly different forms of the ERTP virtualization that already landed. I did a trial rebasing locally and the only remaining change were in packages/run-protocol/ as expected. I'll glance at those changes, but I'll hold off on a review of the real PR until I know I'm looking at the right code.

@erights erights force-pushed the markm-virtualize-vpool branch from aa07f79 to 8bdf6f4 Compare March 23, 2022 03:38
@erights
Copy link
Member Author

erights commented Mar 23, 2022

@erights could you rebase this and force-push a new version? It looks like this branch includes slightly different forms of the ERTP virtualization that already landed. I did a trial rebasing locally and the only remaining change were in packages/run-protocol/ as expected. I'll glance at those changes, but I'll hold off on a review of the real PR until I know I'm looking at the right code.

Done. But, given @Chris-Hibbert 's comments at #4707 (comment) I'm putting this back into draft status. Instead, it seems #4706 is closer to what we want, so I'm declaring #4706 Ready to Review. Everyone, please review that one instead.

@erights erights marked this pull request as draft March 23, 2022 03:55
@erights
Copy link
Member Author

erights commented Mar 23, 2022

@FUDCo @warner Please look at https://github.com/Agoric/agoric-sdk/runs/5654712229?check_suite_focus=true
Before rebasing, this PR was green under CI. After rebasing I see

virtualObjects › virtualObjectGC › VO refcount management 3 faceted

  Difference:

  - undefined
  + {
  +   key: 'vom.rc.o+11/3',
  +   result: '1',
  +   type: 'vatstoreGet',
  + }

  › validate (packages/SwingSet/test/liveslots-helpers.js:259:7)
  › voRefcountManagementTest3 (packages/SwingSet/test/virtualObjects/test-virtualObjectGC.js:1297:3)
  › async packages/SwingSet/test/virtualObjects/test-virtualObjectGC.js:1332:3

which looks like a GC bug.

@FUDCo
Copy link
Contributor

FUDCo commented Mar 23, 2022

@FUDCo @warner Please look at https://github.com/Agoric/agoric-sdk/runs/5654712229?check_suite_focus=true Before rebasing, this PR was green under CI. After rebasing I see

virtualObjects › virtualObjectGC › VO refcount management 3 faceted

  Difference:

  - undefined
  + {
  +   key: 'vom.rc.o+11/3',
  +   result: '1',
  +   type: 'vatstoreGet',
  + }

  › validate (packages/SwingSet/test/liveslots-helpers.js:259:7)
  › voRefcountManagementTest3 (packages/SwingSet/test/virtualObjects/test-virtualObjectGC.js:1297:3)
  › async packages/SwingSet/test/virtualObjects/test-virtualObjectGC.js:1332:3

which looks like a GC bug.

This has to be some kind of merge error in the rebase. Don't know what it is, but something got lost in translation.

@turadg
Copy link
Member

turadg commented Apr 22, 2022

Done in #5187

@turadg turadg closed this Apr 22, 2022
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.

5 participants