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

Return offer result in makeOffer #122

Open
samsiegart opened this issue Jan 12, 2025 · 7 comments
Open

Return offer result in makeOffer #122

samsiegart opened this issue Jan 12, 2025 · 7 comments
Labels
enhancement New feature or request

Comments

@samsiegart
Copy link
Contributor

What is the Problem Being Solved?

The makeOffer function in walletConnection.ts doesn't share the offer result, it only reports error, accepted, or refunded. Apps might want to access the offer result after making an offer, and right now they'd have to manually watch updates in vstorage to see them.

Description of the Design

Add a case for update.status.result !== undefined in watchUpdates, it could do something like:

onStatusChange({ status: 'result', data: update.status.result });

One consideration to figure out is when to stop watching for updates. Does every offer return a result or an error? Is a result guaranteed to be returned before or after numWantsSatisfied?

Test Plan

Probably testing by linking the package locally with dapp-offer-up would be the most straighforward.

@samsiegart samsiegart added the enhancement New feature or request label Jan 12, 2025
@amessbee
Copy link

@samsiegart this seems easy enough to fix. I was wondering if there is reason, we don't want to pass along the whole update.status or even update?

@samsiegart
Copy link
Contributor Author

@samsiegart this seems easy enough to fix. I was wondering if there is reason, we don't want to pass along the whole update.status or even update?

We could, it's just not what I had in mind when I wrote the function. I was intending it to abstract away parsing the update with numWantsSatisfied etc. to make it clear what the state of the offer is, and exit the function when the offer is completed. There's no guarantee result will be returned before numWantsSatisfied. Also, putting publicSubscribers in the result is the recommended way to publish data related to an offer, and makes it readable via publicSubscribersNotifier, which is how dapp-inter currently works with user's opened vaults.

@amessbee
Copy link

@samsiegart I tried a simple example via a patch here:

It seems that update.status.return is set to the string UNPUBLISHED and not the returned object. I am not sure what I am doing wrong to get the actual object.

@samsiegart
Copy link
Contributor Author

It seems that update.status.return is set to the string UNPUBLISHED and not the returned object. I am not sure what I am doing wrong to get the actual object.

See offerWatcher.js#L192. If you pass a copyrecord it will show as UNPUBLISHED. Did you mean to link a commit? I don't see the patch you're talking about

@amessbee
Copy link

Oh sorry, I meant to link this:
agoric-labs/dapp-ed-cert#6

@amessbee
Copy link

@samsiegart Looking at offerWatcher.js#L192, I don't see a way to return anything that is not a primitive type from offerHandler. Is that correct?

@samsiegart
Copy link
Contributor Author

I don't see a way to return anything that is not a primitive type from offerHandler. Is that correct?

You can, it will just show up in the status as UNPUBLISHED. You could try serializing the return value to a string if it's serializable.

Otherwise, note the if statement here, it will save the result if it contains invitationMakers, and also publish the paths of the publicSubscribers. See this example contract. This is the continuing invitation pattern.

If you don't use the continuing invitation pattern, the offer result won't be saved, so it can be overwritten in future status updates for the same wallet. So, anything important should use that pattern. I'm not sure the use case for needing to read a non-saved offer result. It would just provide transient feedback in the UI, but I'd imagine the result should be inferable based on the offer status and the behavior of the contract.

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

2 participants