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: Add missing labels for crypto to fiat and refactor bridgeXYZMutation #3778

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Nov 26, 2024

Description

This PR addresses multiple issues:

  • the label shown for the 3rd column Routing number for USD accounts and BIC for EUR accounts
    Screenshot 2024-11-28 at 19 25 33
    Screenshot 2024-11-28 at 19 25 58

  • the status pill and call-to-action default values if the retrieved kycStatus is null or empty
    Screenshot 2024-11-28 at 19 30 40

  • improving the error handling when an invalid liquidation address is created by Bridge by using isAddress from ethers and returning a null value if the check is false

  • aaaaand a bit of refactoring of the bridgeXYZMutation lambda

Testing

TODO: So let's also test these changes locally, though the focus on this PR review should be more related to the code refactoring.

  • Step 1. Make sure you add .env files to the bridgeXYZMutation lambda

Important

It should have the following content - please ping me for the values if you don't already have them

BRIDGE_API_URL=
BRIDGE_API_KEY=
ENV=dev
  • Step 2. Now connect as leela and go to http://localhost:9091/account/crypto-to-fiat
    Screenshot 2024-11-28 at 19 37 57

  • Step 3. Enter the personal details, go to the next step, close the modal and refresh the page - you should be able to add you Bank details now
    Screenshot 2024-11-28 at 19 40 18

  • Step 4. Add an USD account (you can use this https://randommer.io/routing-number-generator to generate some values, though the Routing number needs an extra digit)

  • Step 5. Check that the Routing number column is displayed
    Screenshot 2024-11-28 at 19 44 24

  • Step 6. Add an EUR account and now check the column label gets updated to BIC
    Screenshot 2024-11-28 at 19 45 05

  • Step 7. Now if you check the amplify logs you'll see a message similar to

No liquidation address found for account, creating one
Newly created liquidation address <SOME_ADDRESS_HERE> is not valid

but the details still show up on the page, beside the liquidation address

  • Step 8. Now if you refresh the page, a liquidation address should show up that was actually created on Bridge, but we didn't store it as it isn't a valid address

Screenshot 2024-11-28 at 19 56 23

The thing is, on DEV, Bridge will generate liquidation addresses that are not considered to be valid - utils.isAddress will return false, hence this value will not be stored.
Before the changes on this PR, we directly called

      utils.getAddress(
        externalAccountLiquidationAddress,
      )

that threw an error in case of an invalid address, causing the UI to no longer display any information without a page refresh. Now this should be fixed, just that the liquidation address will not be displayed without a page refresh

  • Step 9. Now in checkKYCHandler at line 32 add throw new Error() and refresh the page
  • Step 10. You should see the following screen
    Screenshot 2024-11-28 at 19 54 35
    compared to master
    Screenshot 2024-11-28 at 19 55 11

Diffs

Changes 🏗

  • bridgeXYZMutation lambda refactoring
  • use utils.isAddress for checking the liquidation address before storing it

Resolves #3442
Resolves #3496

Refactor: bridgeXYZMutation lambda
Fix: Add missing routing number label
Refactor: Add generic request helpers for rest calls
@mmioana mmioana self-assigned this Nov 26, 2024
Refactor: Move consts
Fix: Add default badge and cta props for crypto to fiat page
@mmioana mmioana changed the title Fix/3442 3496 crypto to fiat Fix: Add missing labels for crypto to fiat and refactor bridgeXYZMutation Nov 27, 2024
@mmioana mmioana marked this pull request as ready for review November 28, 2024 17:07
@mmioana mmioana requested a review from a team as a code owner November 28, 2024 17:07
@mmioana mmioana marked this pull request as draft November 28, 2024 17:07
@mmioana mmioana marked this pull request as ready for review November 28, 2024 18:59
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

We tested it together on a call against Bridge production and confirmed everything still works as expected, including:

  • getting KYC status
  • updating bank details
  • drains history
  • creating liquidation addresses

Nice refactoring effort, thank you for taking the time to make it look prettier 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps operations.js would be a better name, similarly to how it's called in domain balance lambdas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants