Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

(Feature) - 1029 Improve Polling behaviour #1047

Merged
merged 28 commits into from
Jul 6, 2020
Merged

Conversation

Agupane
Copy link
Contributor

@Agupane Agupane commented Jun 23, 2020

Closes #1029

Description

  • Adds exponential-backoff dependency for implementing backoff in case that a request fail
  • Also converts all the "fetch methods" to singleton to avoid call an API before the last request finish
  • Dependency updates
  • replaces direct usage of web3.utils.toChecksumAddress with an util checksumAddress
  • Refactor useCheckForUpdates to use setTimeout instead of setInterval and rename it to useSafeScheduledUpdates
  • add types for safe reducer and selectors (Not all, but already helped to catch a few bugs)
  • Specify a default value for UPDATE_SAFE action

We could probably in another pr improve this and maybe try the library that @nicosampler suggested :) but at least for now I think it solves our current problem

@Agupane Agupane self-assigned this Jun 23, 2020
@Agupane Agupane requested review from mmv08 and fernandomg June 23, 2020 12:20
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

…29-improve-polling

# Conflicts:
#	yarn.lock
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

src/logic/collectibles/sources/OpenSea.ts Outdated Show resolved Hide resolved
src/logic/collectibles/store/actions/fetchCollectibles.ts Outdated Show resolved Hide resolved
src/logic/tokens/store/actions/fetchSafeTokens.ts Outdated Show resolved Hide resolved
src/logic/collectibles/sources/OpenSea.ts Outdated Show resolved Hide resolved
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

2 similar comments
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

…to 1029-improve-polling

# Conflicts:
#	src/routes/safe/store/actions/fetchSafe.ts
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@mmv08 mmv08 self-assigned this Jul 3, 2020
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

2 similar comments
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@Agupane Agupane requested review from fernandomg and removed request for fernandomg and nicosampler July 6, 2020 15:12
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1047--safereact.review.gnosisdev.com/app

@Agupane Agupane merged commit 26a358a into development Jul 6, 2020
@Agupane Agupane deleted the 1029-improve-polling branch July 6, 2020 19:34
})
}

const incomingTransactions = await loadIncomingTransactions(safeAddress)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this bot using the backoff?

dispatch(addIncomingTransactions(incomingTransactions))
export default (safeAddress: string) => async (dispatch: Dispatch): Promise<void> => {
try {
const transactions = await backOff(() => loadOutgoingTransactions(safeAddress))
Copy link
Member

Choose a reason for hiding this comment

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

I would set a maximum delay of like 5 or 10 min else this will completely break the ux if the service is down for a couple minutes.

Copy link
Member

Choose a reason for hiding this comment

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

it will be called again

Copy link
Member

Choose a reason for hiding this comment

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

it fetches the data continuously, if this request fails, the interface will send a new one

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but it increases the delay (*2) so at some point this will be a very long delay (not critical as it can be reset with a reload :) )

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Actually looking at it the default values are not the best for oir use case. 100ms as a default delay makes not too much sense in my opinion as this will only triggered more failed request really fast. I would probably set this to 5 or 10 seconds. And in this case 10 retries with infinitive delay is quite a lot (~1.5h delay with 5 seconds)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve polling behaviour
7 participants