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

optimistic view claim rendering #290

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Aug 30, 2024

This should make the UI appear quicker because we are no longer awaiting the transaction to be confirmed onchain before allowing the tx be viewed on a block explorer. The loading mechanic once the wallet prompt is confirmed should disappear almost instantly as we have the tx.hash already.

  • render the view claim button before we await the tx
  • still produce the success toast after we await the tx

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Aug 30, 2024

Copy link
Contributor

github-actions bot commented Aug 30, 2024

@Keyrxng Keyrxng requested a review from 0x4007 August 30, 2024 10:13
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

I can't test from mobile. Perhaps somebody else can

static/scripts/rewards/web3/erc20-permit.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Sep 6, 2024

@Keyrxng @whilefoo auto merge and self assign plugins need to only accept collaborator reviews not contributor. Can you guys address this

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 7, 2024

I can't test from mobile. Perhaps somebody else can

I tried earlier but idk what I'm looking for honestly (and I messed it up and used prod).

Could you attempt to detail exactly where the bottlenecks you experience are whenever it happens again please as it would make it slightly easier to think of other fixes or if this is going to make any difference, thanks.

  • time to load everything prior to clicking claim
  • After clicking claim but before metamask has popped up
  • Once metamask prompt has been confirmed
  • Time between tx sent and tx confirmed

@0x4007
Copy link
Member

0x4007 commented Sep 8, 2024

  1. Claim button press to metamask prompt
  2. ...until transaction complete but perhaps that depends on the blockchain speed (optimistic rollup, such as Optimism, UX will probably be best)

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Green toast is misleading. Let's just add a new info, grey, toast for as soon as the transaction is initiated to share the transaction link.

Then retain the original behavior of showing green once it's completed.

@ubiquity-os ubiquity-os bot merged commit 3850fef into ubiquity:development Sep 11, 2024
3 checks passed
@0x4007
Copy link
Member

0x4007 commented Sep 12, 2024

@Keyrxng This shouldn't have been merged. Can you fix?

@gentlementlegen
Copy link
Member

Need ubiquity-os-marketplace/daemon-merging#19 in to fix this problem.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 12, 2024

Green toast is misleading. Let's just add a new info, grey, toast for as soon as the transaction is initiated to share the transaction link.

Then retain the original behavior of showing green once it's completed.

I was confused by this originally but look at my local notifications. The logic is as you describe, the notifications arrangement wasn't changed, previously we showed the link after the tx resolved and just before the success toast. Now we show the link after the tx is fired but before we await it.

image

The first info call is made inside transferFromPermit() then we display the link to the explorer and then inside waitForTransaction() we make the success call.

@Keyrxng Keyrxng mentioned this pull request Sep 12, 2024
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