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

Handle governor warnings similarly to NTT queue #2831

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

artursapek
Copy link
Collaborator

@artursapek artursapek commented Oct 11, 2024

Addresses #2830

Adapts the existing transfer delay warning logic to also cover GovernorLimitWarning with a hard-coded 24 hour period.

Copy link

netlify bot commented Oct 11, 2024

👷 Deploy request for wormhole-connect pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 31eb5a0

Copy link

netlify bot commented Oct 11, 2024

👷 Deploy request for wormhole-connect-mainnet pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 31eb5a0

/>,
);
}

for (const warning of quote?.warnings || []) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

props.chain ? `to ${props.chain} ` : ''
}may be delayed due to rate limits set by ${
props.token
}. If your transfer is delayed, you will need to return after ${duration} to complete the transfer. Please consider this before proceeding.`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is an AutomaticTokenBridge transfer and it gets delayed by the governor, the transfer will be relayed once the VAA is emitted 24 hours later. The user doesn't need to return to complete it manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this copy is ok. Might be too complicated/confusing to have different copy for manual and automatic.

Copy link
Contributor

@aadam-10 aadam-10 Oct 15, 2024

Choose a reason for hiding this comment

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

I think the text should say "after at most 24 hours", because enough room may free up in the rolling window before 24h (unless the tx is over the big tx limit but I guess we don't need to complicate this iteration)

as for later, I believe Wormholescan is able to supply the ETA when a tx is scheduled to be released so future iterations can provide better UX by telling the user how much they'll need to wait.

Copy link
Collaborator

@Wesleyleung Wesleyleung Oct 17, 2024

Choose a reason for hiding this comment

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

imo having a clear and consistent expectation (24 hours) even if its conservative is better than something that is not. As a user, I would be fine just checking back exactly the next day and seeing it done one time, vs thinking it might be done less than 24 hours and repeatedly checking in at random times throughout the day to be disappointed. If it happens to be done sooner and I check in sooner, great, then expectations underpromised overdelivered.

@Wesleyleung
Copy link
Collaborator

@emreboga for governor warnings or NTT warnings, would these show up as history as "in-progress"? Or in the widget as well?

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.

4 participants