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

Add retries to ENV #1039

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

abdullah-almesbahi
Copy link

@abdullah-almesbahi abdullah-almesbahi commented Dec 14, 2023

Motivation and context

I work at Badger DAO and we are using this library in our Github action's workflow, we occasionally encounter inconsistent behavior. While it often executes as expected, there are instances where we face the following error:

[switchToMetamaskNotification] Max amount of retries to switch to metamask notification window has been reached. It was never found.

Does it fix any issue?

Increase retries helped us to make Github action's workflow always works for us

Quality checklist

  • I have performed a self-review of my code.

@duckception
Copy link
Contributor

Hey! Thanks for the PR. This is the first time I've heard about this issue 🤔

With what values of RETRIES_SWITCH_TO_METAMASK_NOTIFICATION and RETRIES_WAIT_TO_BE_HIDDEN are you running your CI?

@abdullah-almesbahi
Copy link
Author

We have set RETRIES_SWITCH_TO_METAMASK_NOTIFICATION=100 in our .env file, but we're still using the default setting for RETRIES_WAIT_TO_BE_HIDDEN. I've added instructions for others who might want to change this default value.

@duckception
Copy link
Contributor

Sorry for the long response time. I'm down to merge the change to RETRIES_SWITCH_TO_METAMASK_NOTIFICATION. Adding RETRIES_WAIT_TO_BE_HIDDEN makes no sense at all so it should be removed. What's the motivation behind CONFIRM_TRANSACTION_RETRIES_LIMIT? 120s is already high enough imo but I might be missing some important context here.

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