-
Notifications
You must be signed in to change notification settings - Fork 13
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
WT-1727 Update smart checkout output interfaces #924
Conversation
* @property {SuccessStatus | FailedStatus | InsufficientFundsStatus} status - The status of the action. | ||
* @property {Array<SmartCheckoutSufficient | SmartCheckoutInsufficient>} smartCheckoutResult - The result of the smart checkout. | ||
*/ | ||
export type ActionResult = { |
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 can't remember if we discussed this in your types review meeting but ActionResult seems like a very generic name which doesn't give much context as to what it is describing. I wonder if this is worth reviewing?
I think we can reduce the layers here by making a BuyResult, SellResult and CancelResult.
Then further down the orders type won't have to be this | that | that
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 raised this but dont think I got much of an opinion on it - but I agree its super generic so I'll update this
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.
Great work @imx-mikhala really nice polish 🎉
@@ -131,13 +131,14 @@ export const SmartCheckoutForm = ({ checkout, provider }: SmartCheckoutProps) => | |||
setLoading(true); | |||
|
|||
try { | |||
checkout.smartCheckout( | |||
const result = await checkout.smartCheckout( |
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.
Oh nice, I noticed the await in my current branch, def helps with the loading state!
asset: { | ||
balance: BigNumber.from(10), | ||
formattedBalance: '10', | ||
fundingItem: { |
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.
This is looking good! 👍
{ | ||
gasFee: { | ||
estimatedAmount: BigNumber; | ||
token?: TokenInfo; | ||
}; | ||
bridgeFee: { | ||
estimatedAmount: BigNumber; | ||
token?: TokenInfo; | ||
}; | ||
totalFees: BigNumber; | ||
} |
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.
Do we have a type for this object?
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, yea we do - just pushed update
|
||
/** | ||
* Interface representing the parameters for {@link Checkout.sell} | ||
* @property {Web3Provider} provider - The provider to use for the sell. | ||
* @property {Array<SellOrder>} orders - An array of sell orders to execute. | ||
* Only currently actions the first order in the array until we support batch processing. | ||
* Currently only actions the first order in the array until batch processing is supported. |
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.
Perhaps say only 'processes' the first order to be inline with batch processing
token: { | ||
...requiredBalance.token, | ||
...required.token, | ||
}, |
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.
fix
fundsRequired: { | ||
amount: required.balance, | ||
formattedAmount: required.formattedBalance, | ||
}, |
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 this be the delta?
Summary
Updates smart checkout, buy, sell and cancel endpoints to align to this doc