feat: [DetailsV2] instant buy #6599
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6599 +/- ##
==========================================
- Coverage 60.18% 59.92% -0.26%
==========================================
Files 719 721 +2
Lines 21143 21176 +33
Branches 6974 6981 +7
==========================================
- Hits 12724 12689 -35
- Misses 8342 8408 +66
- Partials 77 79 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
src/nft/hooks/useFetchAssets.ts
Outdated
| @@ -100,3 +106,48 @@ export function useFetchAssets(): () => Promise<void> { | |||
| tokenTradeInput, | |||
| ]) | |||
| } | |||
|
|
|||
| export const useFetchSingleAsset = () => { | |||
There was a problem hiding this comment.
A lot of this code is duplicated from useFetchAssets. Have you tried when a users hits the buy button to add it to the bag, keep the bag hidden, and then just call useFetchAssets()? This would pair well with the updated designs for handling price changes as then you'd just have to reveal the bag.
There was a problem hiding this comment.
The follow up would be if a user already has items in their bag. In that case you could add an optional asset? param to useFetchAsset which would override using the assets currently in the bag.
There was a problem hiding this comment.
I think the surface area between those two functions are actually very low there are only a few lines of code that they share in similiarity and the useFetchAssets function deals a lot with the bagstate which this should not touch. Also this has different outputs, design has decided that if the item is unavailable or if there is a price change to send it over to the bag. This would result in a flag being added to the fetch query and you would basically have two different functions running in one, so I really think it would be best just to separate it out. Also if the useFetchAssets query changed this doesn't need to change I don't see any reason they need to be coupled other than calling the same functions both are calling
There was a problem hiding this comment.
This function would still need to touch bag state in the case of a price change since we'll need to display that change in the bag to the user to review. If we leverage useFetchAssets that logic is already handled for us. The only substantial change for the fetch query would be a conditional on nftTrades: singleAsset ? buildNftTradeInput([singleAsset]) : buildNftTradeInputFromBagItems(itemsInBag), the rest of the logic should be able to be left untouched.
There was a problem hiding this comment.
I am currently working on handling updated price in bag with a prompt - when that pr comes in I will be in a better place to see if we can merge the two functions until then I propose separate and if it looks like they can be combined I will
There was a problem hiding this comment.
this seems like it is returning a callback to purchase the asset (and fetch related data). so it seems like useBuyAssetCallback might be a better name? otherwise i would think this function just gives me the data for the asset
1 flaky tests on run #11173 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Swap > Swap on main page > swaps ETH for USDC |
Output
Screenshots
Video
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
There was a problem hiding this comment.
unable to 1 click buy pooled assets with no price change, see #6599 (comment)
|
|
||
| return ( | ||
| <> | ||
| <StyledBuyButton disabled={isLoadingRoute} onClick={() => fetchAndPurchaseSingleAsset(asset)}> |
There was a problem hiding this comment.
could go either way here but you could useCallback
There was a problem hiding this comment.
I am a little confused by this comment it is using a callback - wondering if you could explain more as to what you would like me to do here
There was a problem hiding this comment.
the onClick function takes a function, which you create new every render. you could use useCallback to keep the function referentially equal if you want, but since this is a simple component and unlikely to change, it's a bit of a wash
src/nft/hooks/useFetchAssets.ts
Outdated
| @@ -100,3 +106,48 @@ export function useFetchAssets(): () => Promise<void> { | |||
| tokenTradeInput, | |||
| ]) | |||
| } | |||
|
|
|||
| export const useFetchSingleAsset = () => { | |||
There was a problem hiding this comment.
this seems like it is returning a callback to purchase the asset (and fetch related data). so it seems like useBuyAssetCallback might be a better name? otherwise i would think this function just gives me the data for the asset
Implementing single purchase asset for nft details. Design has not figured out the way to handle price changes yet or assets that happen to be unavailable so right now we just return and don't execute a metamask transaction.
Automated testing