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

introducing new vbank assets results in O(n) work to update smart wallets #6652

Closed
1 task done
Tracked by #6689
dckc opened this issue Dec 8, 2022 · 3 comments
Closed
1 task done
Tracked by #6689
Assignees
Labels
bug Something isn't working cosmic-swingset package: cosmic-swingset vaults_triage DO NOT USE wallet

Comments

@dckc
Copy link
Member

dckc commented Dec 8, 2022

Describe the bug

As noted in a Dec 5 comment on #6625, when proposal 17 to add DAI passed and 2 new vbank issues were added, each smart wallet woke up, added a purse, and published its new state to vstorage:

// watch the bank for new issuers to make purses out of
void observeIteration(E(bank).getAssetSubscription(), {

This is clearly not consistent with our Limit work by message size design constraint.

To Reproduce

Steps to reproduce the behavior:

  1. Add several hundred wallets
  2. call E(vbank).addAsset(...)
  3. see hundreds of purses created and hundreds of vstorage updates

Expected behavior

TBD, but something that respects our Limit work by message size design constraint.

RPC clients should be able to get the list of vbank brands without relying on updates to individual wallets:

design discussion: IBIS: How to introduce new VBank Issuers to Wallets - Google Docs

Platform Environment

mainnet. pismoA

@dckc
Copy link
Member Author

dckc commented Dec 20, 2022

Failing test

ea697a5 cc @turadg @arirubinstein

~/projects/agoric-sdk/packages/smart-wallet$ yarn test test/test-addAsset.js 
...
  ✔ [expected fail] avoid O(wallets) storage writes for a new asset
    ℹ {
        base: {
          wallets: 2,
          writes: 4,
        },
        test: {
          wallets: 8,
          writes: 16,
        },
      }

@dckc
Copy link
Member Author

dckc commented Jan 13, 2023

I coded up lazy purse creation in #6774 . But another option that's starting to look good is

Skip chainStorage writes for vbank purse balances

Now that we have agoricNames.vbankAssets (#5762 ), clients can just subscribe to queries from x/bank and join with vbankAssets on denom to get brand, displayInfo.decimalPlaces, etc. The smartWallet need not track purse balances; it can just use purses for the duration of handling an offer.

This would update our approach to

It would be a significant change to the wallet front end. And we would need another approach when we add non-vbank assets (LP tokens etc.)

I'm not sure how the REPL wallet API would work either.

@dckc
Copy link
Member Author

dckc commented Feb 8, 2023

Let's call this fixed in #5412
and consider #6807 part of #6788

@dckc dckc closed this as completed Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cosmic-swingset package: cosmic-swingset vaults_triage DO NOT USE wallet
Projects
None yet
Development

No branches or pull requests

3 participants