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

fix: custom rfox reward address display #7734

Merged
merged 10 commits into from
Sep 18, 2024
Merged

fix: custom rfox reward address display #7734

merged 10 commits into from
Sep 18, 2024

Conversation

NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Sep 11, 2024

Description

If you add a custom reward address to rfox, it's not displayed and it would be overriden by

Since we introduced a copy button, we know see an empty copy button:
image

We should display the actual address instead

To solve most of the issues, we decided with the product team to remove the address override feature from the staking feature, here is how it should behave:

  • If you never staked

    • Multi account wallet: Display the account dropdown enabled
    • Single account wallet: Display the account dropdown disabled
    • Not supporting rune: Display a text telling that no address is up, let the user the choice to use a custom address
  • If you staked already

    • Multi account wallet: display the account dropdown disabled
    • Single account wallet: display the account dropdown disabled
    • Not supporting rune: display the custom address in a tag with a copy button
    • Custom address: display the custom address in a tag with a copy button

Issue (if applicable)

closes #7694

Risk

Low

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

On rFOX:

  • If you never staked

    • Multi account wallet: Display the account dropdown enabled
    • Single account wallet: Display the account dropdown disabled
    • Not supporting rune: Display a text telling that no address is up, let the user the choice to use a custom address
  • If you staked already

    • Multi account wallet: display the account dropdown disabled
    • Single account wallet: display the account dropdown disabled
    • Not supporting rune: display the custom address in a tag with a copy button
    • Custom address: display the custom address in a tag with a copy button

Also verify that the Change Address component is working as expected (be able to change the address in any ways using any wallet)

Also if you have an active staking, you could be able to click change address on the top right of the bottom part to go to the change address tab

Engineering

n/a

Operations

n/a

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

MM without snap and without staking active:

image

MM With custom address and staking active

image

MM with snap without active staking

image

Native with custom reward address

image

Native with internal account active staking

image

Native without staking active

image

@NeOMakinG NeOMakinG requested a review from a team as a code owner September 11, 2024 11:14
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally, confirmed this does what it says on the box:

https://jam.dev/c/7b79745e-b9db-4c29-b9d0-e7632a7fd598

Noticed two bugs while testing this however:

  • When having a custom RUNE rewards address, it is impossible to continue without manually inputting said address again. This is confirmed to be a bug against develop, though safer, as the previous behaviour for multi-accounts was automagical <AccountDropdown /> select, which would change the user's reward addy
  • When having a custom RUNE rewards address, the <AccountDropdown /> does not display for multi-accounts wallet, neither in Stake nor Change Address. We should let the users toggle between Use Custom Address or Use Wallet Account (though happy to have this one done in a follow-up)

@NeOMakinG
Copy link
Collaborator Author

Tested locally, confirmed this does what it says on the box:

https://jam.dev/c/7b79745e-b9db-4c29-b9d0-e7632a7fd598

Noticed two bugs while testing this however:

  • When having a custom RUNE rewards address, it is impossible to continue without manually inputting said address again. This is confirmed to be a bug against develop, though safer, as the previous behaviour for multi-accounts was automagical <AccountDropdown /> select, which would change the user's reward addy
  • When having a custom RUNE rewards address, the <AccountDropdown /> does not display for multi-accounts wallet, neither in Stake nor Change Address. We should let the users toggle between Use Custom Address or Use Wallet Account (though happy to have this one done in a follow-up)

Talked with the product team:

If a user already has a stake:

  • On the stake view, show the reward address as readonly mode, change the "Use custom address" link to "Change address" and redirect to the change address tab on click
    -- On the change address tab, let the dropdown displayed as usually and custom address input, we don't care anymore about the preselected address

  • If the user didnt stake yet, show the actual component

@NeOMakinG NeOMakinG marked this pull request as draft September 12, 2024 12:08
@NeOMakinG NeOMakinG marked this pull request as draft September 12, 2024 12:08
@NeOMakinG NeOMakinG marked this pull request as ready for review September 12, 2024 12:18
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally, while this does the do in happy paths, spotted two regressions:

Native

Already staked accounts (e2e, including wallets switch)

While this does what it says on the box for the most part, noticed two issues here, one which is a blocker and the other which is more like a UI improvement:

  • UI improvement: it is possible to copy the account dropdown addy in "Change Address", but not the actual rewards address

Screenshot 2024-09-12 at 18 09 47

  • I was able to get into a non-actionable confirm button, which I believe may be the the same root cause as before. i.e, if you were to be able to manually input the address here still, I think this would work?

https://jam.dev/c/9779abe3-e831-4260-baf2-afa18f777836

Never staked account

  • Most likely unrelated to this PR, I was never able to continue as fees estimation seem to be failing for some reason when coming back to input and going to confirm again, but the json-rpc call definitely succeeded here

image

https://jam.dev/c/5036d7aa-8a8d-4a19-aef7-a12237e92148
  • Tested back fresh after a refresh, and could repro again

MM

Already staked account

  • Could repro the same regression re: disabled state

https://jam.dev/c/46f1d57a-3b45-4b6d-ab77-a49916724cd5

Never staked before account

  • Widget goes hasta la vista after "Confirm and Stake" click as stakeTxid is undefined, so unable to test this

image

Confirmed develop is happy with the same flow:

image

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

One last spotted regression, weird case of a missing address next to a copy button but otherwise LGTM

Never staked before account e2e (MM) :forbidden:

https://jam.dev/c/c731fff4-1b96-4551-b5bb-304c1d831e05

  • I was able to stake succesfully this time around
  • Staking again doesn't work

Also noting that the copy button is still borked in "Change Address" here and displaying no addy:

image

@NeOMakinG we should also add custom address support in useRfoxStake(), so that we always pass a runeAddress down, otherwise fees estimation will be disabled, meaning that we'll be perma-disabled:

image

Already staked account e2e (native) :check

https://jam.dev/c/655634d1-c298-4aae-9c67-90f8bb6b2874

Noting the addition of custom reward addy copy button 🎉

Native account switch (e2e) :check:

https://jam.dev/c/25751e0e-5299-490b-8052-6640bc09cde7

Already staked account e2e (MM) :check:

Weird one since this didn't work for a newly staked account

https://jam.dev/c/790d8f2b-d3cc-4559-a4b3-f509e2decf29

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

@NeOMakinG tested with MM (no multichain snap) and native, LGTM, stamping this one for the sake of progression, as this makes things better than before - tyvm for the patience on the review cycles here! See testing below, three spotted issues to be tackled as follow-up:

  • Native "Same address not allowed" state missing
  • "Change Address"'s " copy button still sad for MM missing copy alongside it
  • Shouldn't be able to toggle "Wallet Address" in change address if there is no THORChain support for the wallet (e.g MM without snaps)

Never staked before account (MM)

@NeOMakinG still noticing two issues re: change address, though non-blocking:

  • Current reward address is not shown

image

  • This copy shouldn't say "Use Wallet Address" - we shouldn't be able to toggle, only custom address is supported for MM and this should say something along the lines of "Use different address"

image

First stake and first subsequent stake

https://jam.dev/c/c03b4544-abed-4048-90b7-d7ad672c0289

Another subsequent stake

https://jam.dev/c/6a2fd5ae-ce63-403c-90d8-185af34ce7db

Never staked before account (native)

https://jam.dev/c/d9687ca3-c1f1-4166-93c6-1787a4df2de3

  • Noticing we're missing error state here for "Same address not allowed":

image

  • Which is happy after triggering address back and forth to the same account:

image

Already staked account (MM) - effectively the same as the previous one, but after a refresh

https://jam.dev/c/c591ba89-c62d-4d24-86b5-7490e347e24b

Already staked account (native)

https://jam.dev/c/f37ea45b-2200-468b-b9aa-0774e1f1f445

@gomesalexandre gomesalexandre enabled auto-merge (squash) September 17, 2024 20:00
@gomesalexandre gomesalexandre merged commit 90454aa into develop Sep 18, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the rfox-address branch September 18, 2024 10:21
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.

RFOX copy button showing up without a RUNE address alongside it
2 participants