support vstorage query by block height

dckc commented 10 months ago

What is the Problem Being Solved?

Description of the Design

briefly: use x-cosmos-block-height as in

https://github.com/endojs/playground/blob/dd07158623eae7319b704723b9467e3df1b2fdb4/packages/ag-trade/src/batchQuery.js#L41-L45

stretch goal: async iterable of results as in readHistory (IIRC, @0xpatrickdev liked that idea)

Security Considerations

Scaling Considerations

Test Plan

samsiegart commented 9 months ago

I think we could add something pretty easily to the queryOnce method to support this. However, if you need an async iterable of results, wouldn't it make sense to use casting instead?

dckc commented 9 months ago

I haven't tried casting lately. I should give it another look.

0xpatrickdev commented 9 months ago

From my perspective, the helpers/functions I think could be useful are -

getOfferResultFromOfferId(offerId, walletAddress) - useful for easily grabbing an offerResult after signAndSubmit()
getOfferResultFromHeight(walletAddress, height, [instance]) - if the user does not include an id in the offer, i think they'd need to do something like this (maybe, we instruct users to always include an offer id, i think it's optional?)
getPreviousOffers(walletAddress, instance, [status]) - allows the ui dev to build "offer history" and "active offers" views

Reading back, get could maybe be replaced with use to follow ui conventions. These will likely use queryOnce under the hood, but these are the ways I anticipate UI devs needing to interact with wallet state. Maybe can include getInvitations(walletAddress, [instance]) in the list.

dckc commented 9 months ago

This issue is specifically about the vstorage layer. The wallet abstraction is a layer above. Your comments seem to be about a wallet query API.

From my perspective, the helpers/functions I think could be useful are -

getOfferResultFromOfferId(offerId, walletAddress) - useful for easily grabbing an offerResult after signAndSubmit()

I think that approach turned out to be buggy: an error could be reported between the time you submit the offer and the time you get around to asking for the result. The correct approach is to start watching for updates before submitting an offer.

In general, the design is for wallet vstorage clients is:

start by getting published.wallet.${address}.current
subscribe to updates to published.wallet.${address}

getOfferResultFromHeight(walletAddress, height, [instance]) - if the user does not include an id in the offer, i think they'd need to do something like this (maybe, we instruct users to always include an offer id, i think it's optional?)

it's not optional.

getPreviousOffers(walletAddress, instance, [status]) - allows the ui dev to build "offer history" and "active offers" views

It's straightforward to get that by filtering by instance. A helper that does that is straightforward.

The filter is at the wallet layer, not the vstorage layer. In ag-trade, I built the wallet layer on top of the vstorage layer like this:

    history: async (visitor, minHeight) => {
      const history = vstorage.readHistoryBy(
        s => query.fromCapData(JSON.parse(s)),
        `published.wallet.${addr}`,
        minHeight,
      );
      for await (const record of history) {
        await E(visitor).visit(record);
      }
    },

-- https://github.com/endojs/playground/blob/main/packages/ag-trade/src/smartWallet.js#L106

Reading back, get could maybe be replaced with use to follow ui conventions.

In the on-chain world, there's a general recognition that a one-time getX thing is rarely what you want; usually you want getXnotifier() (or is it subscriber? I'm not clear). That maps neatly to event steams in FRP style UI state management. Working with baconjs was the last time where it was really clear to me how that worked. Is there a convention for FRP streams in react?

samsiegart commented 9 months ago

dapp-econ-gov has some such history features and makes use of casting and an archive node to achieve this (see: usePublishedHistory). But, archive nodes are more scarce, and dapp-econ-gov is slow to load when doing many queries to build history. I'm not sure if it makes sense to add features to ui-kit that expect an archive node and a slow chain of queries. A backend service that can use casting, its own archive node, and cache results might be more appropriate for most applications.

ui-kit currently has makeOffer which accepts an onStatusChange param for responding to success/failure/exit.

Also, the smart wallet contract stores and publishes offer results that include publicSubscribers or invitationMakers, for when you need to do something or watch something related to the result of that offer. You get a map of offer ids to public subscribers with publicSubscribersNotifier without needing to go back through block history.

For "in-progress" offers, these are published in published.wallet.${address}.current under the key liveOffers. The wallet-app uses the chainStorageWatcher directly to watch for these without relying on block history here. But, it has to do some wrangling to update their displayed statuses when they exit. We can add some more convenient functionality for this in the wallet-connection component without relying on block history.

I think this covers a lot of cases, but not all. It would be better to cover as many cases as possible without relying on archive nodes, blockheight concerns, and long chains of queries on page-load.

Agoric / ui-kit