-
Notifications
You must be signed in to change notification settings - Fork 214
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
Conversation
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. |
this looks relevant to #8862 |
There was a problem hiding this 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
5465c20
to
ce6e703
Compare
ce6e703
to
241c2ca
Compare
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.