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

PR: retry mint for a failed payment card order #357

Merged
merged 11 commits into from
Nov 24, 2024

Conversation

EresDev
Copy link
Contributor

@EresDev EresDev commented Nov 12, 2024

Resolves #291

In issue specs, I suggested retrieving the transaction hash of the partially failed mint by reading the events of permit2. I tried that. Even though permit2 has events, but none of those events are for the claim. Because of this, the events of permit2 are of no use to us here.

In the current solution, we store the permit nonce and tx hash in local storage and use it in retry if the mint failed previously. This is good enough for us.

However, there is one drawback of this solution. When minting, the user has to provide two confirmations using their web3 wallet. One for the permit transaction, and the second to sign a message. Moreover, the signing of message also provides an additional benefit. /post-order is open to public and anyone can call it if the transaction has partially failed but transferred the permit amount to card treasury. It was very useful if the best card for user was fixed. But the best card for user can change based on its location and availability. This adds risk as public can mint a wrong card. Signing of message removes this risk. Only actual user can retry mint after looking at currently available card for them.

QA

QA-partially-failed-minting-3.mp4

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Nov 12, 2024

@EresDev
Copy link
Contributor Author

EresDev commented Nov 13, 2024

In issue specs, I suggested retrieving the transaction hash of the partially failed mint by reading the events of permit2. I tried that. Even though permit2 has events, but none of those events are for the claim. With this, the events of permit2 are of no use to us here.

In the current solution, we store the tx hash in local storage and use it in retry if the mint failed previously. This is good enough for us.

Copy link
Contributor

github-actions bot commented Nov 13, 2024

Preview Deployment
53b57dd2934787c02e228bd0b47d6cefb57ddca8

@EresDev EresDev requested review from 0x4007, Keyrxng and rndquu November 14, 2024 19:29
@EresDev EresDev marked this pull request as ready for review November 14, 2024 19:30
Copy link
Contributor

ubiquity-os bot commented Nov 18, 2024

@EresDev, this task has been idle for a while. Please provide an update.

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.

Code seems fine. What's the best way to test it?

functions/post-order.ts Outdated Show resolved Hide resolved
static/scripts/rewards/gift-cards/mint/mint-action.ts Outdated Show resolved Hide resolved
@EresDev
Copy link
Contributor Author

EresDev commented Nov 20, 2024

Code seems fine. What's the best way to test it?

@0x4007
Best way to test is using Reloadly production but it will cost money as it will give you a card at the end. You can also test it using Reloadly sandbox. It is decided in wrangler.toml file where you fill in either sandbox or production API credentials.

Steps

  • clone the resposiroty
  • fill in the .env [50$ permit for reloadly sandbox, or 7$ permit for reloadly production]
  • fill in the Reloadly API credentials in wrangler.toml
  • yarn install
  • yarn build && yarn start

It will give you a permit url that you can use to mint a payment card.

If you want to test using local anvil fork, make sure you fill the correct anvil info in .env and you can start anvil using

  • yarn test:anvil
  • add anvil rpc printed on console to metamask.
  • yarn test:fund

Test cases

You can perform the following test cases:

  1. mint a payment card
  2. mint a payment card after a failed attempt. Here you have to cause somehow the /post-order request to fail. Many things are in control of Reloadly here. One way is to block the request to /post-order. So that other blockchain requests go through but post order is blocked. This will generate the ideal case where permit amount is now in treasury but payment card has not been minted. To retry, relaunch the page but this time, do not block "/post-order". It should mint a card.

@rndquu rndquu requested review from rndquu and removed request for rndquu November 20, 2024 07:37
@rndquu
Copy link
Member

rndquu commented Nov 20, 2024

@EresDev

  1. Could you add a unit test for a successfull "partial mint" (i.e. when /post-order fails at first but then user calls it again and the mint is successfull)?
  2. Check this line which retrieves reloadly order transactions for the last year. If, for example, user mints a card on 01.01.24 and then tries to mint the card again using the same signature on 01.01.25 (1 year later) then this check will be passed. Question is: Is it possible to create an order transaction in reloadly API with the same order id?

@EresDev
Copy link
Contributor Author

EresDev commented Nov 21, 2024

Could you add a unit test for a successfull "partial mint" (i.e. when /post-order fails at first but then user calls it again and the mint is successful)?

Unit tests are for backend. Retrying mint is not a backend feature, but a frontend feature. You probably see signed message validation in backend and we have unit tests for it included with this PR. For, retry mint, we can have an e2e test, but before we add that our e2e tests are false positives at the moment and should be fixed here. Overall, I think it is not very important to have an e2e test for retry mint. (more unit test but less e2e tests, plus retry mint isn't that critical op)

Check this line which retrieves reloadly order transactions for the last year. If, for example, user mints a card on 01.01.24 and then tries to mint the card again using the same signature on 01.01.25 (1 year later) then this check will be passed. Question is: Is it possible to create an order transaction in reloadly API with the same order id?

This is a good catch. Yes, it seems possible to with same order id to order again after a year. I have added a fix for this. The reloadly api endpoint requires both the start and end date to check transactions for duplicates. I am now using start of epoch 1970-01-01 00:00:00 as a start date and it should remove the possibility of duplicate orders.

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Works fine

Copy link
Contributor

ubiquity-os bot commented Nov 24, 2024

@EresDev, this task has been idle for a while. Please provide an update.

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.

Code looks fine

@0x4007 0x4007 merged commit 53b57dd into ubiquity:development Nov 24, 2024
4 checks passed
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.

Add retry mechanism for partially failed virtual card order
3 participants