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: claim permit in gift cards #226

Merged
merged 101 commits into from
Jun 17, 2024
Merged

PR: claim permit in gift cards #226

merged 101 commits into from
Jun 17, 2024

Conversation

EresDev
Copy link
Contributor

@EresDev EresDev commented May 10, 2024

Resolves ubiquity/card-issuance#34

Table of Contents

Introduction

This feature gives a bounty hunter an option to claim their permit reward in a virtual visa/mastercard.
There is no change in the ubiquibot functions. A hunter gets a reward permit on github issue they solve from ubiquibot. You notice a change when you reach the pay.ubq.fi to claim the permit reward. Here, you see the old option to claim in crypto, and a second option to claim a gift card.

Screenshot_20240523_005410

How does it work for the end user (the bounty hunter)?

There is no change in the way reward was claimed in crypto. It works the way it used to.
If a user clicks a "Claim" button for a gift card, they also get a similar transaction confirmation prompt from their web3 wallet. When the user confirms this transaction, and if all goes well. The page reloads, and they or anyone with a link to this permit can see the following page. However, only the actual owner can reveal the redeem code of the purchased card by signing a special message.

Screenshot_20240523_011541

Changes to the architecture of pay.ubq.fi

Before gift cards, we had two components. Frontend and blockchain. Hunters were claiming their permit in crypto only, and you know how these components were interacting.

Now, we have a third component, which is our backend using Cloudflare functions. It is used to buy a gift card from Reloadly using its API and deliver it back to the frontend if the order is successful.

In the process of claiming a gift card, the permit amount is transferred to our GIFT_CARD_TREASURY address instead of the hunter's own address. This happens when we change the permit.transferDetails.to address to GIFT_CARD_TREASURY address when the user clicks to claim a gift card.

If the transfer is successful, we send this transaction hash with some other info to the backend to buy visa/mastercard for the hunter. The backend checks the details of the transfer and makes sure they are correct, not already used, and the correct amounts, etc. You can see what it checks in file https://github.com/EresDevOrg/pay.ubq.fi/blob/development/functions/post-order.ts . If all is well, an order is placed with Reloadly and the hunter is informed that he has claimed the gift card.

After the order, the user or anyone can see on the pay.ubq.fi.... permit page that a gift card has been claimed. Here, only the owner can reveal the gift card code.

All communication with Reloadly happens through our backend. There is no direct communication between the frontend and Reloadly. Because Reloady needs API credentials or an access token with every request, and we can't reveal them to the public.

How does /post-order work?

This is the most important backend function that consumes our Reloadly balance and buys gift card from Reloadly for the user who are claiming their permit in gift cards. This function receives their crypto transaction hash txHash and chainId and productId of required the gift card from the user after they have successfully confirmed the permit transaction for gift card.
chainId is used to connect to a correct RPC (from our specified list) for blockchain eth/gnosis.
There is no user auth here. All is in the txHash that we check, and give the gift card if the checks pass. What we check is exhaustive to make sure no one cheats. What do we check and why do we check in /post-order? You can also see in this function

function validateTransaction(txParsed: TransactionDescription, txReceipt: TransactionReceipt, chainId: number, giftCard: GiftCard) {

  • We connect to a correct RPC, and we fetch transaction and transaction receipt using txHash. If there is no transaction receipt, we send an error to user, because the transaction has not been mined yet. In normal flow, this should not happen as the request is forwarded here after confirmation.
  • We check in the transaction if the amount transferred can cover the required gift card cost.
  • We check if the transaction is an interaction with Permit2 contract and not some other smart contract.
  • We check if permit.transferDetails.to is our GIFT_CARD_TREASURY address.
  • We check if the transaction is a call to permit2 function permitTransferFrom
  • We check if it transfers required ERC20 token and not some other token.
  • We check if the tokens came from the address from where tokens for permits are supposed to come.
  • We check if txHash has not been already used to claim gift card
  • We also make sure here that only tx.from (the user wallet address) can later get the gift card.

If all of these conditions are correct, we buy gift card from Reloadly. Otherwise, an appropriate error is logged and a generic error is sent to the end user.

Addresses like GIFT_CARD_TREASURY, Permit2, Token are present on the constants file. They usually will not change, but if we need to, they can be changed.

What is needed to merge this PR & deploy to production?

  • We need to wait for Support pages functions deployment cloudflare-deploy-action#12 to finish so that our build & deploy system also deploys backend functions.
    Alternate manual option is to deploy using your CLI. The command you will use is npx wrangler pages deploy "static" --project-name "pay-ubq-fi" --branch "development" --commit-dirty=true Please use appropriate --project-name here.
  • Add env secrets to Cloudflare pages project.
RELOADLY_API_CLIENT_ID 
RELOADLY_API_CLIENT_SECRET
  • Add env variable to Cloudflare pages project USE_RELOADLY_SANDBOX=false
  • Add RELOADLY_SANDBOX_API_CLIENT_ID and RELOADLY_SANDBOX_API_CLIENT_SECRET to GitHub secrets so that the cypress tests could run. These e2e tests place orders on Reloadly sandbox.
    Currently, you see cypress tests failing in this PR. It is because of these variables. You can see the same cypress tests passing on the source repository because it has these variables.
    Also, there is a security concern that these keys can leak somehow from cypress tests workflow, but I decided to keep them because they are just sandbox keys. It should not affect the production.
    RELOADLY_SANDBOX_API_CLIENT_ID: ${{ secrets.RELOADLY_SANDBOX_API_CLIENT_ID }}
  • Make sure all of these constants are correct. I tried my best to use the correct addresses. This is important for the security of the transaction check when placing an order for a gift card. https://github.com/EresDevOrg/pay.ubq.fi/blob/development/shared/constants.ts
  • add cloudflare functions server IP to Reloadly IP whitelist
  • recharge Reloadly with money, initially not big amount

How do I run & try it locally?

Here, we use Foundry Anvil to fork gnosischain.

giftCardTreasuryAddress
permitTokenOwner
  • nvm use
  • yarn install
  • yarn build
  • yarn test:anvil
  • yarn test:fund
  • yarn start will generate permits and start wrangler to serve the backend. Go to the recently generated permit link and try.
  • In your web3 wallet, add anvil RPC, and the beneficiary account you have filled in .env file to claim the permit.

What can be improved next?

  • The app should be moved to React or some other framework. It is getting hard to extend and manage the dynamic html.
  • Maybe add a little fee to gift card claims so that it covers the server (cloudflare) fee if there is any.

@EresDev EresDev self-assigned this May 10, 2024
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented May 10, 2024

@0x4007
Copy link
Member

0x4007 commented May 13, 2024

I only had the chance to quickly look through the code from my phone. Are you using Cloudflare Pages Function due to authentication?

Otherwise I'm curious why you couldn't run everything from the UI?

Tag my name when you reply so that I get a push notification. Thanks!

@EresDev
Copy link
Contributor Author

EresDev commented May 14, 2024

Are you using Cloudflare Pages Function due to authentication?

Correct. This is the main reason. The API keys and access key of Reloadly give almost full access to a user to place an order. They can purchase a gift card of bigger amounts than their reward if there is no backend to keep a check on that. In fact, anyone can place an order, doesn't matter if they have a reward permit or not, if they have those keys.

Plus, I think with only frontend, you can't check pre-conditions to place orders reliably. The payment of gift card is done correctly, it has been mined. The transfer is to/from the correct addresses. All of these things can be manipulated by a bad actor on the frontend. @0x4007

@EresDev
Copy link
Contributor Author

EresDev commented May 14, 2024

I also want to bring your attention to how the pricing of these cards works to make sure we deduct the correct amount and you are okay with it. Because the way Reloadly puts price on these cards is a little complicated.

  • There is a fixed pricing, gift cards are available in 10$, 30$ ... etc exact amounts.
  • There is range pricing, some cards are available 10-100$, any amount.

On top of this, there is an extra fee per purchase, there is a percentage fee on each purchase, and then there is a discount.
We need to account for all that. Right now I am adding all fees in card price, and deducting the discount. So end user gets all that automatically.
Plus, another thing that is making calculations tough is currency difference. A card of 10Euro value, it has a price that keeps changing because of exchange rates as our account is in USD, I am currently working on this part to make it more reliable.

I see that I need to use the permit in one go. Let the user buy card that is available in the exact amount as permit. I tried and the permit doesn't appear to allow a second transaction even if you use some of it.

@0x4007
Copy link
Member

0x4007 commented May 15, 2024

I tried and the permit doesn't appear to allow a second transaction even if you use some of it.

That's a shame.

Regarding the card mint flow, my vision is to have a deposit contract for Ubiquity Dollars. We can use the blockchain as our database. Given the complications with the card amounts, we can do the math on the UI, and then have a button that says "deposit $73" etc or whatever is required, then the button will trigger a transfer of ubiquity dollars from the users wallet to the deposit contract. Once the transaction is complete, the UI can use a wallet signature to expose the card details to the user?

We should be leveraging the built-in security properties of web3 for authentication etc.

Regarding euros etc: let's only mint USD cards. No need to complicate things in euros? Or are you saying there is no USD option?

@EresDev
Copy link
Contributor Author

EresDev commented May 15, 2024

then have a button that says "deposit $73" etc or whatever is required, then the button will trigger a transfer of ubiquity dollars from the users wallet to the deposit contract.

Currently, it is very much like claiming permit. You claim card, instead of claiming crypto.
As you said, asking user to transfer ubiquity dollars, we will also have to invalidate the permit. It increases the steps for the user. A better approach would be to use ubiquity dollars at the permit level instead of wxdai, and this gift card feature will continue to work hopefully without any major change.

We can use the blockchain as our database.

I have found a way to handle everything we need without a database. Reloadly allows one custom field to be saved with each order. I store a hash of

gift card buyer wallet address + permit signature

I can retrieve the order later using these values.

Once the transaction is complete, the UI can use a wallet signature to expose the card details to the user?

Correct, this is already implemented and is working nicely.

We should be leveraging the built-in security properties of web3 for authentication etc.

Yes, we are doing so. I am storing the order info/card numbers, and showing only to the correct user using hashing and signatures.

Regarding euros etc: let's only mint USD cards. No need to complicate things in euros? Or are you saying there is no USD option?

This simplifies my work because this is where I am working now. Most of the other stuff is done.
Why is it complicated? Reloadly left some weaknesses in their API. When currency is different (our account USD, but gift card in EUR or CAD), reloadly starts to mix prices in two decimals and upto 4-5 decimals. I need to get to that exact number of card value and card price to be sure what I am doing is right and using mixed decimals doesn't help. Reloadly also doesn't explain how all these fees and discounts are added, nor it provide an API to figure out the exact amount it will deduct for a card. So I need to figure this out myself.

Secondly, it needs a separate http request to fetch each exchange rate. So when you are showing a list of 10 to 100 gift cards, it is a problem.
I think what I can do is provide USD cards for now, and in the future sometimes we can add non-USD cards too. Not having non-USD cards will be missing out on a lot of opportunities.

@0x4007
Copy link
Member

0x4007 commented May 16, 2024

A better approach would be to use ubiquity dollars at the permit level instead of wxdai, and this gift card feature will continue to work hopefully without any major change.

Yes we will want to provide incentives for our partners to settle in Ubiquity Dollars, such as the ability to mint payment cards!

gift card buyer wallet address + permit signature

What about a wallet signature instead? Their public wallet address seems less secure because it's public information.

Secondly, it needs a separate http request to fetch each exchange rate. So when you are showing a list of 10 to 100 gift cards, it is a problem.

I want to spend extra time thinking through the UI/UX so that the user is never bombarded with a view of tens of cards. We should do our best to abstract/hide this away. Perhaps we can check their balances and if they are too low then we automatically hide them and save the state in localStorage etc.

Another UX flow to consider: I imagine that as soon as I mint my cards, I will immediately add it to my Apple Pay wallet. After that I do not need to access the card details from the UI (unless if I can see balance details, then this is useful) otherwise I will only use the card from my Apple Pay wallet app and a copy will be saved there.

Also last thing that's really important: we should have the ability to "roll up" permits into a single card. For example, if I have five permits that total up to $900, it would be far better UX to mint a single $900 card instead of five small cards. Please let me know your plan for addressing this.

Not having non-USD cards will be missing out on a lot of opportunities.

I understand that USD cards can be spent anywhere. @sergfeldman minted me a $10 USD card and I spent it using Apple Pay in South Korea at a convenience store without a problem.

@EresDev
Copy link
Contributor Author

EresDev commented May 16, 2024

I have a question. I am about to test on Ethereum. I have never tried an Ethereum permit here. It appears everything on Ethereum is also the same as gnosischain.

ERC20 DAI token is used to pay on Ethereum with permits? @0x4007

@0x4007
Copy link
Member

0x4007 commented May 16, 2024

ERC20 DAI token is used to pay on Ethereum with permits? @0x4007

Technically, any token should work, but yes, in the past when we first started the system we settled in DAI on mainnet

@sergfeldman
Copy link

I understand that USD cards can be spent anywhere. @sergfeldman minted me a $10 USD card and I spent it using Apple Pay in South Korea at a convenience store without a problem.

Yes, as far as I know, you can pay everywhere with a card in USD.
The foreign transaction fee is acceptable.
For example, from approximately 9 USD the fee of 0.17 USD was paid.
More details can be found in the card transaction history
TransactionHistory.pdf

EresDev added 3 commits June 18, 2024 02:09
fixes broken test
Signer will be different partners addresses. No benefit in checking.
@EresDev EresDev merged commit 48fa5d0 into ubiquity:beta Jun 17, 2024
3 checks passed
@0x4007
Copy link
Member

0x4007 commented Jun 17, 2024

Missed opportunity here, but we should have made a card icon like so for card minting
card

@EresDev
Copy link
Contributor Author

EresDev commented Jun 17, 2024

@EresDev actually now that I merged the other pull there are some merge conflicts. I hope that you can resolve these and then you can merge it right after. Thank you.

I have resolved the merge conflicts and I have merged it. Also, I have added a few more fixes.
I have given a list of steps for deployment (including beta). I think some of these can only be done by you as you have the access. They are mostly saving the environment vars. You should go through them. #226 (comment) @0x4007

@0x4007
Copy link
Member

0x4007 commented Jun 17, 2024

@EresDev actually now that I merged the other pull there are some merge conflicts. I hope that you can resolve these and then you can merge it right after. Thank you.

I have resolved the merge conflicts and I have merged it. Also, I have added a few more fixes. I have given a list of steps for deployment (including beta). I think some of these can only be done by you as you have the access. They are mostly saving the environment vars. You should go through them. #226 (comment) @0x4007

@rndquu is a super admin on the Ubiquity Workers Cloudflare account and should be able to address

@EresDev
Copy link
Contributor Author

EresDev commented Jun 17, 2024

Missed opportunity here, but we should have made a card icon like so for card minting

No problem. I am expecting some UI change requests. So let them gather up and I will implement them.

I believe clicking this will show cards below. I had this icon in the same spot at some point during implementation. This mingles the UI code of claiming gift card and crypto. Right now, the UI codes are very much separate.

@Keyrxng
Copy link
Member

Keyrxng commented Jun 17, 2024

I have tested using polygon amoy and a WXDAI mock deployment, claiming to my own address and signing using a vanity-eth address

I run it e2e on mobile and desktop without errors so everything appears to be working as expected. I did notice that on mobile the cards lose their background and are difficult to see. They appear this way without interaction I just killed two birds capturing the toast.

image


I also think that the additional info is uneasy on the eyes

image


I only tested with the sandbox creds but functionally things are working as expected and I think all of the angles have been covered in terms of it being exploitable, great work.

Idk if it's due to my region or if everyone is only showed one card now I thought the intention was to show multiple but I was only able to see one so I'm unsure how multiple would look on mobile.

I was testing from 68dfa23

@0x4007
Copy link
Member

0x4007 commented Jun 17, 2024

I will spend some time working on the UI/UX whenever its set up on beta.pay.ubq.fi etc

Very excited to demo this.

@EresDev
Copy link
Contributor Author

EresDev commented Jun 17, 2024

Idk if it's due to my region or if everyone is only showed one card now

Unfortunately, Reloadly has only 1 visa gift card, and no mastercard on Sandbox. So, everyone will see only one card. You can see more cards if you use the production API keys of Reloadly. But those will be real gift card and claiming them will actually buy the cards. But I think it is okay to test claiming using sandbox, and test UI using production API keys. Claiming can be tested on production at the end of all other testings.

@Keyrxng
Copy link
Member

Keyrxng commented Jun 17, 2024

The carousel looks and feels good on mobile as far as I can tell

@rndquu
Copy link
Member

rndquu commented Jun 18, 2024

@EresDev So I should:

  1. Set RELOADLY_API_CLIENT_ID, RELOADLY_API_CLIENT_SECRET to production values in cloudflare env variables
  2. Set USE_RELOADLY_SANDBOX=false in cloudflare env variables
  3. Set RELOADLY_SANDBOX_API_CLIENT_ID, RELOADLY_SANDBOX_API_CLIENT_SECRET to sandbox values in github secrets for this repository

Frontend + functions deployment will be handled by deploy.yml.

Correct?

@EresDev
Copy link
Contributor Author

EresDev commented Jun 18, 2024

@EresDev So I should:

  1. Set RELOADLY_API_CLIENT_ID, RELOADLY_API_CLIENT_SECRET to production values in cloudflare env variables
  2. Set USE_RELOADLY_SANDBOX=false in cloudflare env variables
  3. Set RELOADLY_SANDBOX_API_CLIENT_ID, RELOADLY_SANDBOX_API_CLIENT_SECRET to sandbox values in github secrets for this repository

Frontend + functions deployment will be handled by deploy.yml.

Correct?

Yes, correct. But please note that we are merging to beta branch, and not development branch. The pages project in cloudflare will be beta-pay-ubq-fi for now.

@rndquu
Copy link
Member

rndquu commented Jun 18, 2024

@EresDev So I should:

  1. Set RELOADLY_API_CLIENT_ID, RELOADLY_API_CLIENT_SECRET to production values in cloudflare env variables
  2. Set USE_RELOADLY_SANDBOX=false in cloudflare env variables
  3. Set RELOADLY_SANDBOX_API_CLIENT_ID, RELOADLY_SANDBOX_API_CLIENT_SECRET to sandbox values in github secrets for this repository

Frontend + functions deployment will be handled by deploy.yml.
Correct?

Yes, correct. But please note that we are merging to beta branch, and not development branch. The pages project in cloudflare will be beta-pay-ubq-fi for now.

There is no beta-pay-ubq-fi project in cloudflare dashboard right now, probably because of the failed https://github.com/ubiquity/pay.ubq.fi/actions/runs/9555228242/job/26338006121 which, as far as I remember, should've created a new project

Could you fix the deploy cloudflare CI and run it again so that a new project beta-pay-ubq-fi is created?

@EresDev
Copy link
Contributor Author

EresDev commented Jun 18, 2024

Yes, correct. But please note that we are merging to beta branch, and not development branch. The pages project in cloudflare will be beta-pay-ubq-fi for now.

@rndquu Deployment to cloudflare is working fine now. However, I think I was wrong in the above statement. I saw @0x4007 creating new project so I thought it will be deployed to new project on cloudflare. @0x4007 do you know how it suppose to deploy to new project or maybe you were just doing this for your own testing. Because if the branch is different, the deployment will still go to the same old project but will be treated a preview deployment and will get a separate URL. If I am right here, you can add the environment variable to the same old pay project. But @0x4007 will tell us more if deployment is expected in a separate project.

@0x4007
Copy link
Member

0x4007 commented Jun 18, 2024

I plan to expose it at some subdomain or some other domain for testing purposes. Ultimately it is a convenience but either way, there will be a direct *.pages.dev link that we can access for testing. Let's focus on getting the *.pages.dev link up and running first, and worry about mapping it to some vanity domain like beta.pay.ubq.fi later.

@rndquu
Copy link
Member

rndquu commented Jun 18, 2024

@EresDev Right now the beta branch is deployed in a "Preview" environment. I've added RELOADLY_API_CLIENT_ID, RELOADLY_API_CLIENT_SECRET and USE_RELOADLY_SANDBOX to the "Preview" environment variables (and also set github secrets).

The latest deployment of the beta branch (with all env variables set) is available at https://c0c9d712.pay-ubq-fi-bxp.pages.dev/

Screenshot 2024-06-19 at 00 42 09

@rndquu
Copy link
Member

rndquu commented Jun 18, 2024

@0x4007 I have plain admin access to the "Ubiquity DAO Workers" project while editing DNS settings requires super admin.

Screenshot 2024-06-19 at 00 44 13

@0x4007
Copy link
Member

0x4007 commented Jun 18, 2024

I modified the global subdomain router to handle what we need, and it seems to be fine, except now the final problem is that somebody is using the https://pay-ubq-fi.pages.dev/ namespace which we need for the router to dynamically map correctly. Right now we have a -bxp suffix that cloudflare automatically adds to avoid collision. Whoever owns the space please must release it. I am looking through my accounts now to see if I can release it.

https://beta.pay-ubq-fi-bxp.pages.dev/

@Keyrxng
Copy link
Member

Keyrxng commented Jun 18, 2024

I may have had it but I spent like 10 mins clearing all of my workers/pages yesterday so if it was me I'd expect it to have propagated by now

@0x4007
Copy link
Member

0x4007 commented Jun 18, 2024

Cloudflare support told me @Steveantor has it!

@0x4007
Copy link
Member

0x4007 commented Jun 19, 2024

We may have to rebrand/use a different subdomain as I did not immediately hear back.

If this is the case, then we must change the bot permit generation code to reflect the new subdomain name @gentlementlegen

Perhaps rewards.ubq.fi

@0x4007
Copy link
Member

0x4007 commented Jun 19, 2024

Just got the namespace back!

photo_2024-06-19 17 57 51

@0x4007
Copy link
Member

0x4007 commented Jun 19, 2024

Couldn't figure out how to make my sub sub domain catch yet (beta.pay.ubq.fi.) Will work on it later. For now this is accessible at https://beta.pay-ubq-fi.pages.dev

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.

Bounty proposal: integrate Reloadly API
5 participants