feat: react components #83
Conversation
| if (isCancelled) return; | ||
| if (!entries) continue; | ||
| const result = Object.fromEntries(entries); | ||
| setOfferIdsToPublicSubscribers(result); |
There was a problem hiding this comment.
Do we want to expose a filter here? It is likely a consumer is concerned with a subset of publicSubscribers versus all of them
There was a problem hiding this comment.
I'm not sure what the best filter would be. We could add one for offerId but the dapp would only know the offer id if the offer was made in the same session. dapp-inter watches all of the public subscribers then picks the ones that have a key called vault, so we could filter by whether they have a certain key, but as commented it's not super reliable. Eventually the smart wallet should probably expose something about where the offer came from, like a board id or agoric name, which would probably be the most useful.
If we decide something would be useful, it wouldn't be too hard to add to src/lib/hooks, it could look something like usePurse
There was a problem hiding this comment.
Ok cool, agree there doesn't seem to be an elegant solution currently.
I think adding useOffers(selectorFn) or useContinuingOffers(selectorFn) with an example for vaults would be a nice addition.
| for await (const result of subscribeLatest(n)) { | ||
| if (isCancelled) return; | ||
| if (!result) continue; | ||
| setPurses(result); |
There was a problem hiding this comment.
I forget how subscribeLatest works. Will it only return if there are new values, or will it still jitter on an interval? If the latter, we may want to add a lodash/isEqual check and only set state if false
There was a problem hiding this comment.
subscribeLatest doesn't check values, if the notifier is updated then it will iterate, even if result is identical. However, the notifier is managed by makeAgoricWalletConnection, and it does indeed seem to be triggering updates when it shouldn't. I think the issue should be fixed there, so I just filed a bug: #85
If we fix it there this shouldn't be a concern, I can follow up with another PR. It would be difficult to do here because useEffect can't read the latest value without making it a dependency, which would reset all the watchers every time.
|
|
||
| const setNetworkConfig = (newConfig: NetworkConfig) => { | ||
| writeToStorage(newConfig); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Is this needed? It looks like AgoricProvider is dependent on NetworkProvider, so the change should bubble down.
There was a problem hiding this comment.
Yes the change would bubble down, but imo basically all of the state coming from the wallet and chain would need to be cleaned up anyway. It's tricky to do that in AgoricProviderLite because the notifiers could only be cleaned up on the next iteration when the loop is broken, so they'll keep polling the old network until an update happens. And any additional state the app is watching (example) would have to be cleaned up, so it's harder on the developer. This feels ugly but it's simple and safe, maybe there's some easy ways to get around those issues though.
There was a problem hiding this comment.
the notifiers could only be cleaned up on the next iteration when the loop is broken
I might be missing something wrt this comment, but here's an approach i took to clean out state when the network config changed: https://github.com/0xpatrickdev/agoric-vault-collateralization-notifier/blob/d6729c6f43c2042ae783320462818c3e3275999f/web/src/contexts/chain.jsx#L34-L47
| chainStorageWatcher, | ||
| walletConnection, |
There was a problem hiding this comment.
Should we maybe just expose makeOffer and provisionSmartWallet here? What else would a consumer need from walletConnection?
There was a problem hiding this comment.
Yea I was being conservative about what I exposed in my initial pass, but those seem like worthy candidates now that you mention it. Done.
samsiegart
force-pushed
the
react-components
branch
from
3831d11
to
7738350
Compare
| address: chain.address, | ||
| chainName, | ||
| connect: chain.connect, | ||
| chainStorageWatcher, |
There was a problem hiding this comment.
Similar to your earlier usePurse() suggestion - wdyt about a hook for chain storage?
const { data, error } = useChainStorage(path: string, kind: Kind, poll?: boolean)Not crazy about "poll", but thinking is to capture whether queryOnce is used versus having a separate hook. Would think it's best to guide developers towards queryOnce unless they're using frequently updated state (contract instances, asset lists, chain parameters, and any Kind.Children queries are probably candidates for queryOnce)
Edit: Looking at the example you linked, I guess this hook would also need a selectorFn to massage the data.
I would then suggest this:
const { data, error } = useChainStorage(path: string, kind: Kind, { poll?: boolean, selectorFn } )
fixes Agoric/agoric-sdk#8813
fixes Agoric/agoric-sdk#8934