-
Notifications
You must be signed in to change notification settings - Fork 11
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
Allow swapping bridged USDC.e to native USDC via new OpenGSN-enabled swap contract #488
Conversation
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.
Adding a check for the swap targetAmount
would be a good idea.
The other comments are optional and not blocking for this PR, but would still be nice.
So for complete native USDC support, the mainnet swap contract would be missing, and then also a native USDC HTLC contract for atomic swaps?
const inputAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) | ||
.args | ||
.amount; | ||
const targetAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) // eslint-disable-line max-len | ||
.args | ||
.targetAmount; |
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 need to cast here as description
already has the desired type.
const inputAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) | |
.args | |
.amount; | |
const targetAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) // eslint-disable-line max-len | |
.args | |
.targetAmount; | |
const { inputAmount, targetAmount } = description.args; |
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.
Does it for you? In my editor, it does not (the type annotation in line 119 doesn't actually work to narrow description
).
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.
What I could do is
const { inputAmount, targetAmount } = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description).args;
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.
yarn typecheck
has no complaints.
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.
Yes, because the variables are happily assigned any
. Which works, but is not helpful.
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.
Yeah I have the same issue as Sisou, parseTransaction
seems to return a TransactionDescription
which is apparently not assignable to description
's type, thus giving me an error line 120.
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 & clean 👍
const inputAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) | ||
.args | ||
.amount; | ||
const targetAmount = /** @type {PolygonSwapDescription | PolygonSwapWithApprovalDescription} */ (description) // eslint-disable-line max-len | ||
.args | ||
.targetAmount; |
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.
Yeah I have the same issue as Sisou, parseTransaction
seems to return a TransactionDescription
which is apparently not assignable to description
's type, thus giving me an error line 120.
Removes the possibility of sending USDC.e, adds the ability to swap it to USDC.
The combined diff might be confusing, as the requests all look similar and I removed the
transferWithApproval
from the top of the handler and addedswapWithApproval
to the bottom of the handler, which messes up Github's diff calculations. I suggest to view the two commit diffs individually.