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

support vstorage query by block height #80

Open
dckc opened this issue Feb 2, 2024 · 5 comments
Open

support vstorage query by block height #80

dckc opened this issue Feb 2, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@dckc
Copy link
Member

dckc commented Feb 2, 2024

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

@dckc dckc added the enhancement New feature or request label Feb 2, 2024
@samsiegart
Copy link
Contributor

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
Copy link
Member Author

dckc commented Feb 8, 2024

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

@0xpatrickdev
Copy link
Member

0xpatrickdev commented Feb 8, 2024

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
Copy link
Member Author

dckc commented Feb 8, 2024

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
Copy link
Contributor

samsiegart commented Feb 9, 2024

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.

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

No branches or pull requests

3 participants