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

Feature/sc 2672/deploy nftx v3 frontend to base #24

Merged
merged 8 commits into from
May 13, 2024

Conversation

jackmellis
Copy link
Collaborator

No description provided.

we recently added support for OR operators, however if you use an AND operator in the same clause, it would not work.
This fix does some additional work to massage the output into nested and/or clauses in order to create a valid query.

As part of this fix the createQuery method has been refactored to be easier to understand and maintain.
@jackmellis jackmellis requested a review from 0xNumlock May 13, 2024 10:11
default:
break;
}
const [holdings, nextId] = await fetchErc721s({

Choose a reason for hiding this comment

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

Nice, much cleaner.

@@ -273,7 +273,7 @@ export const makeFetchVaultActivity = ({
const activity: VaultActivity[] = [];
const feeDistributorAddresses = [
getChainConstant(NFTX_FEE_DISTRIBUTOR, network),
...getChainConstant(NFTX_FEE_DISTRIBUTOR_LEGACY, network),
...getChainConstant(NFTX_FEE_DISTRIBUTOR_LEGACY, network, []),

Choose a reason for hiding this comment

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

Can't we just make the last param optional instead of passing in empty values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

getChainConstant is a generic fn that we use all over and the return value is also generic, so if you want a fallback value, you have to pass it in (or it returns undefined).

Copy link

@0xNumlock 0xNumlock left a comment

Choose a reason for hiding this comment

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

Awesome, small comment but no blockers - brilliant work as always!

@jackmellis jackmellis merged commit 83609b0 into main May 13, 2024
1 check passed
@jackmellis jackmellis deleted the feature/sc-2672/deploy-nftx-v3-frontend-to-base branch May 13, 2024 12:07
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