notifying smart wallet of every vbank balance update is too expensive

[warner]

What is the Problem Being Solved?

@michaelfig and I were brainstorming about vat-bank improvements.

As mentioned in #5885, our current vat-bank implementation results in some pretty expensive work being done each time any cosmos-managed balance changes (which includes the RUN and IST balances of every account). The cosmos code sends a VBANK_UPDATE message over the bridge with all the updates, then vat-bank updates an in-RAM copy of those balances. This enables vat-bank to provide a full Purse API on the virtual purses it exports to other users.

To avoid that work, we'd really prefer to only use those virtual purses for their .deposit and .withdraw methods, and leave balance checking (both one-shot and ongoing notification) to more efficient pathways. The cosmos Bank module already publishes all of those balances to the IAVL tree, and there are well-established RPC tools to query them (a balance query being practically the first thing that any blockchain implements).

There are three places that are interested in a balance:

• external users (e.g. a user wallet UI): they should be using the RPC tools to fetch these balances
• the on-chain smart wallet: ideally this should not need to query the cosmos-managed balances at all: it knows of their existence, and it tells the user that they exist, but it should not need to be updated with the actual balance number, except maybe when doing a deposit/withdraw
• an ag-solo (perhaps a wallet within an ag-solo), which might be holding a Presence backed by one of those virtual purses.. we're not sure how to address this yet

The on-chain smart wallet currently subscribes to balance updates for all the user's purses, both the real ones and the virtual purses that reflect cosmos-side Bank module balances. It publishes a single aggregate record to the IAVL tree (containing a list of purses and their current balances) every time any one of those balances change.

We're thinking that that record should only include the "real" purses, not the vat-bank -managed virtual purses. When the wallet UI wants to render a list of all the user's purses, it should make two queries: one to the on-chain wallet's aggregate record (for the "real" purses), and then a second to the Bank module's normal balance-publishing path (for the denominations that would be represented by virtual purses). Then, there should be a shared event, a "doorbell" of some sort. The wallet UI/frontend watches for that event, and when it happens, it re-performs both queries. The on-chain wallet code triggers this event each time it changes the aggregate record (because one of the "real" purses has changed value). The cosmos Bank module (or vbank module?) also triggers this event when one of the vpurse balances has changed.

That way, we're not doing any on-chain work in reaction to a cosmos Bank balance changing: we just ring the doorbell so the frontend can know it should re-query over RPC.

We're thinking that the virtual Purses exported by vat-bank be stripped down: we remove both getCurrentBalance (or have it return undefined or something), and we remove the getNotifier too. The on-chain wallet shouldn't be querying these anymore, and off-chain code should know that the data is available through the normal cosmos Bank RPC queries.

@michaelfig I'm sure I've missed a lot here.. could you help fill in the gaps?

cc @samsiegart @dckc

Description of the Design

Security Considerations

Test Plan

[dckc]

Looks good.

I was worried about the 2 queries, since @michaelfig gave me the impression that watching multiple chainStorage nodes requires a websocket each (soon to be an http long-polling thingy each). But the single doorbell addresses that complexity.

[erights]

Without getCurrentBalance and getNotifier, it would no longer implement the EOnly<Purse> type. Having getCurrentBalance return undefined or anything other than a "current" balance would be worse. It would no longer correctly implement the Purse behavior spec.

If we do either, we should stop describing these as Purses. But I don't understand why we need to. getCurrentBalance can ask the cosmos side for the balance. getNotifier is trickier, but supporting this with a cold notifier doesn't seem impossible.

If we do plan to implement them, but just have not yet, then have these methods throw a "not implement yet" error. We could even go to MN-1 with such a promise that we'll eventually implement the full Purse behavioral spec.

Or we could rename these to something other than Purse (or EOnly<Purse>).

[warner]

That all seems fair.

But I'd be concerned if these virtual purses support getNotifier, but it's pretty expensive to actually use: we're leaving a non-obvious performance footgun lying around for others to trip upon. If the on-chain wallet were to use this notifier for balance updates, that would make "cold" notifiers pretty hot, and we'd be back to every account*denomination causing a cosmos-to-swingset bridge message being sent (and a couple of vat deliveries) for every cosmos-side balance update (maybe staking rewards? that sort of thing). "cold" only helps if clients rarely query, but the Notifier pattern (especially the Iterator wrappers) make it really easy to aggressively pull every update.

[erights]

Proposal: How about if we keep the name Purse and continue to plan to implement the full Purse behavioral spec somehow. We implement getCurrentBalance by querying the cosmos side.

For now, we have getNotifier throw a not-yet-implemented error. Once we have satisfied the notification needs of vbank clients other ways, then, possibly after MN1, we implement the postponed getNotifier method.

[turadg]

This issue is referring to Purse methods getCurrentBalance and getNotifier, though I believe we're talking about these:

agoric-sdk/packages/ERTP/src/types.js

Lines 345 to 350 in 7f294a6

	* @property {() => Amount} getCurrentAmount
	* Get the amount contained in this purse.
	*
	* @property {() => Notifier<Amount>} getCurrentAmountNotifier
	* Get a lossy notifier for changes to this purse's balance.
	*

An open question is how to remove the hotness from the vat-bank -managed virtual purses. Some options:

1. make getCurrentAmountNotifier return undefined or null
2. define a new type like Purse but that doesn't have a notifier
3. make getCurrentAmountNotifier throw "not implemented"
4. replace the Notifier with a Subscriber

I haven't heard #4 proposed yet. Wouldn't that solve the "hot" problem?

[erights]

Yes, getCurrentAmountNotifier. Some tentative reactions, fwiw:

1. make getCurrentAmountNotifier return undefined or null

Makes the type more complicated.

2. define a new type like Purse but that doesn't have a notifier

Adds a distinct supertype, which is more explicit and in-band than 1 or 3. IMO probably best.

3. make getCurrentAmountNotifier throw "not implemented"

Does still impose a burden on clients, even if only temporarily. Advantage of 2 over 1 or 3 is that the type system could help clients reason about when they need to deal with this burden.

4. replace the Notifier with a Subscriber

I haven't heard #4 proposed yet. Wouldn't that solve the "hot" problem?

As long as clients are only holding the Iterable over time, rather than the Iterator, should be equivalent, and equivalently hard to cool. Unlike Notifiers, if clients do hold a Subscriber Iterator, hotness becomes unavoidable as it can no longer be lossy. But I do not understand the current similarities and differences of the internal mechanisms of Notifiers and Subscribers, so take with much salt. @gibson042 , reactions?

[warner]

There are multiple kinds of expense here:

• A: the publishing side must remember all history, forever
• B: the cosmos side must tell the vat side about every balance update (even ones nobody is paying attention to)
• C: the cosmos side must tell the vat side about every balance update, but only for ones being paid attention

Using Notifier instead of Subscriber moves us away from expense A. Changing the cosmos side to speak only when spoken to (er, notify only when someone asks) moves us away from expense B.

But, expense C is still pretty bad. Especially if the on-chain smart wallet automatically subscribes for updates for every single user (so it can publish the balances to the storage node that it uses for all the non-virtual purses). We could easily wind up with just as much cosmos<->swingset traffic as expense B. In other words, lazy notification is only a win if it's actually lazy, and eager clients are not lazy.

And, the balances of those VBANK-managed accounts are already in the IAVL tree, and reachable over RPC queries that predate our entire company. So it's a waste of our precious chain CPU time to report those balances into JS just to re-publish them in a second RPC-queryable spot.

If some contract code needs to query it occasionally, that's fine, but presenting a Notifier API on it makes it awfully easy to query it constantly, and I think that represents load that we can't afford.

[erights]

So we only need to avoid getCurrentAmountNotifier. I agree we should. Since this is now a permanent design feature rather than a temporary implementation limit, of the ways of getting rid of it, of @turadg 's list at #5896 (comment) I now clearly favor 2 (new distinct Purse-like type). This would also let us get rid of the need to use the EOnly type modifier, as we could just define the new type directly.

[turadg]

@warner is this a blocker for #5885 ? are either of them necessary for MN-1? (neither currently tagged as such)

[warner]

Like all our important contracts, I worry about the position we'll be in if we ship without full upgradability. If we have that, then we should (hopefully?) be able to upgrade vat-bank into a form that doesn't use the expensive/eager update mechanisms. But if the load caused by those is large enough, that might interfere with our ability to upgrade them in the first place. So.. maybe?

[dckc]

#6652 prompts looking at this again

[dckc]

provisionPool relies on vbank balance notifier

The provision pool responds to incoming cosmos token transfers of, for example, USDC_axl, and trades the incoming assets for IST.

agoric-sdk/packages/vats/src/provisionPool.js

Line 180 in e208b20

observeNotifier(E(exchangePurse).getCurrentAmountNotifier(), {

[michaelfig]

To hit the sweet spot between no balance notifications and all notifications, I plan on sending notifications from Golang only for privileged Cosmos "module account" addresses (like the provisionPool), not for general permissionless account addresses.

[dckc]

Ok... That meets all the requirements I can think of.