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

migrate markets from nftobject #155

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

olegakbarov
Copy link
Contributor

  • Migrating from NFTObject will dramatically simplify codebase and remove dependency on zdk.
  • Key aspect of the task is decoupling of token and markets
  • tokenId, contractAddress are currently "drilled" through number of components, but it makes sense to extract them in provider later on.
  • rewrite of useAskHelper and useRelevantAsk essentially unlocks the migration to NFTObject as those are two main touch points where legacy data types are used

@olegakbarov olegakbarov requested a review from bjfresh December 8, 2022 18:46
@vercel
Copy link

vercel bot commented Dec 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nouns-marketplace ❌ Failed (Inspect) Jan 11, 2023 at 4:41AM (UTC)

@olegakbarov olegakbarov marked this pull request as draft December 8, 2022 18:46
tokenId: string
contractAddress: string
collectionName: string
markets: ReturnType<typeof useToken>['markets']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does typegen generate any types that are a little more intuitive than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My attempt to extract it was unsuccesful. Good thing is that this type is so ugly that it begs for further improvement.

// }
// }

export const useAskHelper = ({ ask }: ReturnType<typeof useToken>['markets'][0]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, think we need more concrete types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree!

)
const rawAskAmount = useMemo(() => ask?.price?.nativePrice.raw, [ask])
const displayAskAmount = useMemo(() => ask?.price?.nativePrice.value, [ask])
const usdAskAmount = ask?.price.usdPrice
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the formatting has been stripped out here. Are we sure output would be correct?

},
// not timestamp in db, only block number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we deal with this?

const auctions = markets.filter(
(item) => item?.properties?.__typename === 'V3ReserveAuction'
)
const asks = markets.filter((item) => item?.properties?.__typename === 'V3Ask')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olegakbarov here, I think we still need to sort with getLatestBlockNumber. This was a fix for a bug that was causing issues during the summer, at least for Asks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for head's up! I assumed sorting from api is sufficient.

(item) => item?.properties?.__typename === 'V3ReserveAuction'
)
const asks = markets.filter((item) => item?.properties?.__typename === 'V3Ask')
const offers = markets.filter((item) => item?.properties?.__typename === 'V1Offer')

const ask = asks[0]
const auction = auctions[0]
const offer = offers[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return all offers and not just offers[0]... all offers are relevant (this is changed in the PR I pushed yesterday... we'd need to merge from main or fix it here)


if (nftObj) {
if (hasActiveAuction) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure all the logic from main is reconciled in the sidebar!

setTimeout(() => revalidate({ retryCount }), 5000)
},
dedupingInterval: 10000,
refreshInterval: 5000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably reduce this frequency

@olegakbarov olegakbarov marked this pull request as ready for review January 9, 2023 23:11
@olegakbarov olegakbarov changed the title [WIP] migrate markets from nftobject migrate markets from nftobject Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants