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

feat: add parcel list modal #398

Merged
merged 9 commits into from
Mar 10, 2023
Merged

feat: add parcel list modal #398

merged 9 commits into from
Mar 10, 2023

Conversation

tnrdd
Copy link
Collaborator

@tnrdd tnrdd commented Mar 9, 2023

Description

Add a modal to list 25 parcels sorted by either highest value, recent claims, random, needs transfer, foreclosure or outstanding bid.

Issue

fixes #392

Checklist:

  • My commit message follows the Conventional Commits specification
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my code
  • My changes generate no new warnings
  • My PR is rebased off the most recent develop branch
  • My PR is opened against the develop branch

Alert Reviewers

@codynhat @gravenp

const foreclosureQuery = gql`
query Foreclosure($skip: Int) {
geoWebParcels(
first: 1000
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to get 1000 parcels here? Or can we limit it to MAX_LIST_SIZE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here and in NeedsTransfer querying for only 25 parcels wouldn't result in 25 parcels foreclosed or that needs transfer because we don't have a way to query the subgraph directly for that, but e.g. need to call isPayerBidActive on each to tell if a parcel is foreclosed.

So we just get a pool of parcels to check and loop over them, performance should be alright even when there are 1000 parcels because it is done asynchronously.
A way to reduce the number of calls to the Ethereum node is to use ethers JsonRpcBatchProvider, in this comment the author was wondering if 1 million calls is too much so 1000 shouldn't be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Great find on the JsonRpcBatchProvider. We should try to limit RPC calls as much as possible. Looks like this is working well with Infura.

I also see Ceramic multiqueries working well and also the Ceramic caching.

This does now result in many IPFS lookups via our delegate node. This should be fine, as they are only calls for content routing and do not return or fetch actual content. However, there are many 404s to /block/stat. I will look into how to remove these calls, as they are unnecessary. findprovs is what is needed.

Copy link
Member

@gravenp gravenp left a comment

Choose a reason for hiding this comment

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

Looks good on my end. Capturing new claims/bids in results where appropriate and performance looks pretty good.

@codynhat can you take a look at the changes @tnrdd made after your comment? Then @tnrdd can merge.

@tnrdd tnrdd merged commit ad373a8 into Geo-Web-Project:develop Mar 10, 2023
@tnrdd tnrdd deleted the feat/390 branch March 10, 2023 18:08
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.

3 participants