Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update MGI guide using TS wallet SDK #658
Update MGI guide using TS wallet SDK #658
Changes from 14 commits
0bd2c8c
2f95447
21649a2
77b4064
d16378e
b8bad48
059eab1
0df20f6
4b55d0b
d61cecf
5125b5d
fe37a4d
8df42f7
a5b0df6
d780a21
055978a
19da0b7
41bdf22
960e7cc
c8dc651
8fccdaa
d45f9d0
295515b
f38f793
1b0fee8
0f45a2d
8dc1c4b
01cdd5f
7d77e8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should follow our own guidelines from SEP-24 guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ifropc this code appears to be broken. We can't access
assetId
directly frominfo.currencies
.I think this is the best we could do with current TS Wallet SDK:
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ifropc I'm wondering if we should display this same code above 👆 in our SEP-24 guide in place of the broken
const asset = info.currencies.find(({ code }) => code === "USDC").assetId;
code. Wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
295515b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We advise against using postMessage approach, I think we should mention that it's being deprecated long term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this, do we even need to recommend it? I thought MoneyGram supported not using postmessage as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the problem of not implementing the
postMessage
handler is that MGI is still displaying a redOK/Close
button on the last screen. So if wallets don't listen for that button it'll behave like a broken button.But one thing I just realized is that Vibrant is not appending this
&callback=postmessage
param to the url but the app still receives thepostMessage
event. So it looks to me that we don't need to ask wallets to append this param, but instead just listen to a postMessage event coming from that last red button so they know it's safe to close the webview and prevent the button from looking broken. What do you guys think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so if we don't listen to it, the button will not work? I think we should describe it in the documentation then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We defined a migration plan with MGI to remove postmessages because of the UX complications in the event that something doesn't work. @Ifropc can you dig up the plans we had and revisit this with MGI? Feel free to loop in the Vibrant team as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks guys, would be nice to remove that button.
Just to clarify: Vibrant does not rely on the postMessage event anymore for a while now, users can close the webview without tapping MGI's red button. Which means MGI would just need to remove that red button and probably add some copy letting users know they can safely close the window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakeUrban they have been stalling the plan for a very long time, it's somewhere in their backlog. We should raise this question again with their team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep the idea is to replace button with the next that says you can close the window now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
960e7cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: can/should we recommend the use of MoneyGram's webhooks now? When this tutorial was originally created they didn't have them. Now I would imagine this the preferred approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
Do we have any existing documentation explaining how to setup/integrate MGI's webhook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so, @Ifropc @nirojapanda are you familiar with any docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry, I don't think there are any docs for that. Their implementation also doesn't match the standard precisely BTW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so do we still want to recommend this approach? If so, can you help create that documentation if you're familiar with the differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's what Vibrant is doing: only listen to postMessage for closing button, and do NOT listen to the message returned by it, @CassioMG am I correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, Vibrant only listen to postMessage for closing the button. But it only does that when the
transaction
that comes inside the event message is in apending
status to avoid closing the webview for ANY other random postMessage event that may happen during the interactive flow for whatever reason.Like this:
Does that make sense?
Sorry about the late response I was OOO these past days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it sounds like using MGI's webhooks instead of postmessages may not be an option yet so we need to continue documenting the postmessage approach. @CassioMG if you think we need to instruct wallets to only close when the status is
pending_user_transfer_start
that is fine by meThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
960e7cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand a bit more into how to use stellarTransactionId or externalTransactionId? Maybe write 3 examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d45f9d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's define on how to query asset above (see another comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
295515b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we write an example on how to handle other errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0f45a2d