GoodDollar / GoodCollective

Monorepo for GoodCollective (Segmented UBI and Direct Payments Pool)
MIT License
3 stars 1 forks source link

added polling when checking if a user is donating to a collective #137

Closed krisbitney closed 5 months ago

krisbitney commented 5 months ago

This should improve the speed at which the app can detect if it should show the option to stop donating

L03TJ3 commented 5 months ago

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy
krisbitney commented 5 months ago

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy

Hmm it should check for new data every 1 second, and only for this query on this page. Are you sure it is increasing the amount of network requests that significantly?

It shouldn't pull data from the subgraph unless there is new data.

The purpose was to resolve this issue: https://github.com/GoodDollar/GoodCollective/issues/136

L03TJ3 commented 5 months ago

@krisbitney compared it against dev-goodcollective.vercel.app. below is from this pr deployment Screenshot from 2024-01-31 17-25-03

L03TJ3 commented 5 months ago

Can this be because of the flowrate and timestamps updating? it keeps fetching data every 5 seconds by default (before this fix)

krisbitney commented 5 months ago

Can this be because of the flowrate and timestamps updating? it keeps fetching data every 5 seconds by default (before this fix)

The flowrate doesn't work by repeatedly fetching. It shouldn't affect it. I'm not sure the cause but I will look into it.

It might just be because of fetching every 1 second vs every 5 seconds. I guess that would be a 5x increase, but it should be only for this one query.

L03TJ3 commented 5 months ago

No I meant because of time stamps updating there is always new data to fetch?

krisbitney commented 5 months ago

No I meant because of time stamps updating there is always new data to fetch?

If you mean this timestamp, it only updates when the smart contract event is triggered. The timestamp refers to that last time the donor-collective relationship was updated. It shouldn't change otherwise.

  query DONOR_COLLECTIVE_BY_ENTITIES($donor: String, $collective: String) {
    donorCollectives(where: { donor: $donor, collective: $collective }) {
      id
      donor {
        id
      }
      collective {
        id
      }
      contribution
      flowRate
      timestamp
    }
  }
krisbitney commented 5 months ago

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy

@L03TJ3 In your screenshot, you got 46 API requests related to the graph. I got a similar number in my test locally.

When testing the master branch locally, I got about 30.

Here are screenshots. The first one is the master branch, and the second is this PR's branch.

Screenshot 2024-01-31 at 6 54 42 PM Screenshot 2024-01-31 at 6 56 24 PM
L03TJ3 commented 5 months ago

Ye I stupidely looked at the wrong digita, apologies

L03TJ3 commented 5 months ago

@krisbitney but besides my wrong analysis about the count of the events. you are saying that it should not keep polling indefinitely correct? so is there then still something wrong with the caching, or?? idk

krisbitney commented 5 months ago

@krisbitney but besides my wrong analysis about the count of the events. you are saying that it should not keep polling indefinitely correct? so is there then still something wrong with the caching, or?? idk

It polls indefinitely, so long as the user is on this page. There is nothing wrong with the caching. The polling is intended to make sure the data this query is fetching gets updates as soon as possible. It checks if there is new data, and only downloads it if it exists.

sirpy commented 5 months ago

@krisbitney @L03TJ3 if we use polling, lets put this as an env variable. 1 second is too much. should be something like 30 seconds. its not that critical to immediately register the user tx

the optimal solution from my POV, is to refetch just once X seconds after the user TX completes

krisbitney commented 5 months ago

@krisbitney @L03TJ3 if we use polling, lets put this as an env variable. 1 second is too much. should be something like 30 seconds. its not that critical to immediately register the user tx

the optimal solution from my POV, is to refetch just once X seconds after the user TX completes

I changed it to an env variable with a default 30 seconds. If it doesn't fix https://github.com/GoodDollar/GoodCollective/issues/136 then you can change the interval or implement Hadar's solution.

decentralauren commented 5 months ago

Was better and will work for now. @patpedrosa @L03TJ3 Created a ticket to revisit modal language to message that things may take a while / encourage patience :)

Closing this issue.