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

feat(ertp): purse amountStore abstraction #8861

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Conversation

erights
Copy link
Member

@erights erights commented Feb 3, 2024

closes: #XXXX
refs: #8862

Description

Abstract the purse's internal storage of its currentBalance into something that encapsulates the actual implementation, and enables that implementation to be, for example, a MapStore implementation of a copyBag amount kind. Currently does not actually change the representation. Just sets us up to do so.

We currently kludge to avoid any upgrade sensitivity, where the actual balance representation is still state.currentBalance: Amount. We don't gain anything until we change that representation. But how should we upgrade old representation instances to use the new store representation?

Also, we don't gain anything until we notify of balance changes rather than new balances. This change by its nature cannot be encapsulated. How do we make that transition?

Security Considerations

none

Scaling Considerations

The whole point is the scaling benefits of changing this internal representation to a store, when warranted. This is an experiment that has yet to actually demonstrate net benefit. This PR only sets the stage, and the mild cost of a few extra heap allocations and function calls. Beyond that, this PR should have no effect on scaling.

Documentation Considerations

Once we do it, will need to document the shift from notifying of new balances to notifying of balance changes.

Testing Considerations

First, do no harm. Until this has observable changes no tests should need to change.

Upgrade Considerations

We need to cope with the external change from balance notifications to balance change notifications.

We will need to upgrade from a state.currentBalance: Amount representation to one using store.

Both will need design.

Another worry is that there are currently 118 occurrences of getCurrentAmount() in agoric-sdk outside of the ERTP package. Any of these that are requests of a store-based-purse will get more expensive, since the purse will need to re-synthesize it from its encapsulated store.

@erights erights self-assigned this Feb 3, 2024
@erights erights marked this pull request as ready for review February 3, 2024 06:58
@erights erights requested a review from dckc February 3, 2024 07:12
@dckc
Copy link
Member

dckc commented Feb 3, 2024

... But how should we upgrade old representation instances to use the new store representation?

Maybe we don't have to?

I expect we could just make new purses and transfer the assets. Purses tend to be closely held. I'd be surprised if any purses are exported from one vat to another (except maybe vbank purses, and those are unrelated to this PR).

Oops... no, the smart wallet keeps invitation purses and KREAd purses. But it could voluntarily make new ones and transfer the assets.

@dckc
Copy link
Member

dckc commented Feb 5, 2024

this looks relevant to #8862

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

some notes from a quick skim

packages/ERTP/src/amountStore.js Outdated Show resolved Hide resolved
packages/ERTP/src/amountStore.js Show resolved Hide resolved
packages/ERTP/src/paymentLedger.js Show resolved Hide resolved
@erights erights force-pushed the markm-ertp-purse-store branch 2 times, most recently from 5465c20 to ce6e703 Compare February 6, 2024 05:58
packages/ERTP/src/amountStore.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-ertp-purse-store branch from ce6e703 to 241c2ca Compare February 12, 2024 19:41
@erights erights added the automerge:squash Automatically squash merge label Feb 12, 2024
@mergify mergify bot merged commit 73fe0af into master Feb 12, 2024
74 of 75 checks passed
@mergify mergify bot deleted the markm-ertp-purse-store branch February 12, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants