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

wallet history: on chain storage vs. update size vs. off-chain searching #6184

Closed
4 tasks done
dckc opened this issue Sep 12, 2022 · 7 comments
Closed
4 tasks done

wallet history: on chain storage vs. update size vs. off-chain searching #6184

dckc opened this issue Sep 12, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request Inter-protocol Overarching Inter Protocol wallet

Comments

@dckc
Copy link
Member

dckc commented Sep 12, 2022

What is the Problem Being Solved?

The following are in tension:

  1. how much state does the wallet store?
  2. how large are chainStorage updates?
  3. how much searching does the off-chain client have to do?

The current contract does well on 1 and 2 at the expense of 3. For example, it produces an update for each new asset/isser/brand. To be sure it has all of them, the client may have to search through all wallet updates back to the first one.

Description of the Design

ideas include:

  • off-chain indexing
  • in each offerStatus update, include the id of the oldest outstanding offer?
  • every N offerStatus changes, publish an aggregate of all balances and brand descriptors

Security Considerations

Test Plan

UIs use .current

  • dapp-econ-gov
  • agoric-cli wallet and psm commands
  • Wallet UI
  • dapp-psm (should be implied by Wallet UI support)

cc @rowgraus

@dckc dckc added enhancement New feature or request wallet needs-design Inter-protocol Overarching Inter Protocol labels Sep 12, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 RC0 milestone Sep 12, 2022
@Tartuffo Tartuffo removed this from the Mainnet 1 RC0 milestone Sep 21, 2022
@dckc
Copy link
Member Author

dckc commented Sep 23, 2022

The current design conflicts with normal pruning practices for RPC nodes.

@turadg is working on econ committee governance, and in devnet, he can't get the full wallet state because nodes are pruning somewhere between blocks 20,000 and 30,000 (current block height is in the 40,000s).

cc @michaelfig @gibson042 @arirubinstein

@gibson042
Copy link
Member

Agreed. We should generally err on the side of publishing complete state information rather than deltas and only deviate upon observing an actual issue.

@dckc
Copy link
Member Author

dckc commented Sep 24, 2022

The actual issues that motivated the current design were:

It's possible that the full state size is now manageable, and we might be able to reduce the 6 updates per trade by 1 or 2, but we'd have to do a bunch of performance testing again.

@turadg
Copy link
Member

turadg commented Sep 25, 2022

publishing complete state information rather than deltas

As a matter of terminology, what we're publishing a third way: "partial states". We've already committed to only publishing current states and never deltas. Delta being the difference between a previous and current state, such as "+3" for a value. Publishing deltas makes it so that no state is observable without accumulating all the deltas. With partial state, what you receive is known to be true, it's just not all the state.

only deviate upon observing an actual issue.

Agreed. As Dan referenced above, there was an actual issue that motivated this. Reducing the updates per trade would increase latency in user feedback on the state of the transaction.

Of course, complete states are preferred. But that isn't that independent of the matter of whether archival can be relied upon or not? Can the smart wallet rely on its RPC nodes to be archival? My understand has been yes, and if that isn't correct we should confer.

@arirubinstein
Copy link
Contributor

@dckc
Copy link
Member Author

dckc commented Sep 26, 2022

@turadg @michaelfig and I chatted about various designs; this one seems to fit the constraints we can think of; @michaelfig is taking the ball.

PreciousWalletStateThingy could perhaps use the lastOfferId high water mark too, to handle a case where a UI is trying to figure out what's going on with an offer in its localstorage.

/**
 * @typedef {{
 *   balances: Amount[],
 *   invitationMakers: Record<number, Amount>,
 *   assets: BrandDescriptor[],
 * }} PreciousWalletStateThingy
 *
 * published.wallet.agoric1123423.current
 */

/**
 * @typedef {{ updated: 'offerStatus', status: import('./offers.js').OfferStatus } |
 * { updated: 'balance'; currentAmount: Amount } |
 * { updated: 'brand', descriptor: BrandDescriptor }
 * } UpdateRecord Record of an update to the state of this wallet.
 *

@turadg
Copy link
Member

turadg commented Sep 30, 2022

Closing and doing dapp-econ-gov separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Inter-protocol Overarching Inter Protocol wallet
Projects
None yet
Development

No branches or pull requests

6 participants