ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)
https://pay.ubq.fi
8 stars 36 forks source link

feat: action workflow to create test URL permits #214

Closed gentlementlegen closed 6 months ago

gentlementlegen commented 6 months ago

Resolves #195

ubiquity-os-deployer[bot] commented 6 months ago
14670c7
59f992f
6220608
db8f0a9
3f4dea0
749469f
41127c7
cb66f57
dfb3849
bc7a531
fe17f75
099c3ba
3c7fbec
b3c9c97
445d779
42acd22
daaf5be
4a8ad27
0359ae8
3f57be8
233afdb
941d461
gentlementlegen commented 6 months ago

A workflow has been added to generate permits that have a value of 0 to any desired wallet. Inputs are:

image

Usable claim payloads are in the build logs:

image

These payloads can be used against the deployed branch, are claimable, usable once and new ones can be generated infinitely, which I think should help for testing. Latest run: https://github.com/gentlementlegen/pay.ubq.fi/actions/runs/8478434201/job/23230754838#step:4:64

0x4007 commented 6 months ago

This approach is decent but the specific problem I was aiming to solve was that I want to be able to test from my phone by clicking the deploy links.

The usability of your implementation is not viable for this use case.

I was thinking we can hard code the deploy action to append the query param for testing purposes.

The app without the passed in permit is pretty useless for testing.

gentlementlegen commented 6 months ago

Of course makes sense. It would be possible to generate the link with the permit and claim in the URL, problem is that it would be only usable once so if it is claimed afterwards you would get a "already claimed" error message. So maybe let's keep this Action for subsequent tests and I'll add it in the build steps so the generated URL also has the claim?

gentlementlegen commented 6 months ago

@pavlovcik I realize that I need a wallet address to generate the permit. Should I default it to yours?

gentlementlegen commented 6 months ago

/query @pavlovcik

ubiquibot[bot] commented 6 months ago
Property Value
Wallet 0x4007CE2083c7F3E18097aeB3A39bb8eC149a341d
0x4007 commented 6 months ago

@pavlovcik I realize that I need a wallet address to generate the permit. Should I default it to yours?

0x0 might be the most professional but there's also the admin wallet ubq.eth 0xefC0e701A824943b469a694aC564Aa1efF7Ab7dd

gentlementlegen commented 6 months ago

So I defaulted it to ubq.eth address to have the UI also behave properly. I manually ran it here https://github.com/gentlementlegen/pay.ubq.fi/actions/runs/8498117002/job/23277489835#step:6:67 The job fails because there is no related issue to post to. I added a step where a comment is added with the full URL to the current issue.

Edit: it seems the URL for the body is wrongly encoded or contains spurious characters at the end, will investigate.

gentlementlegen commented 6 months ago

@pavlovcik an example of the comment can be found here: https://github.com/gentlementlegen/pay.ubq.fi/pull/11#issuecomment-2029594132

Let me know if the formatting is satisfactory. No new comment is created anymore, and you should be able to open the page simply by clicking on the hash which contains the deployed URL with the permit payload.

0x4007 commented 6 months ago

@pavlovcik an example of the comment can be found here: gentlementlegen#11 (comment)

Let me know if the formatting is satisfactory. No new comment is created anymore, and you should be able to open the page simply by clicking on the hash which contains the deployed URL with the permit payload.

Do you have an example of it being appended? I see it made an edit but isn't it more useful to show all of them? Why don't you just take the logic from this? https://github.com/ubiquity/pay.ubq.fi/pull/214#issuecomment-2026809006 source code here.

gentlementlegen commented 6 months ago

The current scenario is that I just update to the latest one because I didn't think it would be relevant to have all the URLs, we are only interested in the final version no? It also avoids doubloons if the workflow is somehow ran twice (two different triggers for example). If that's a bother I can change the logic to have all the deployments.

0x4007 commented 6 months ago

The current scenario is that I just update to the latest one because I didn't think it would be relevant to have all the URLs, we are only interested in the final version no? It also avoids doubloons if the workflow is somehow ran twice (two different triggers for example). If that's a bother I can change the logic to have all the deployments.

We have it implemented this way to ease review and debugging. If a bug is introduced with the latest commit, its trivial to check the previous one with our current approach. This was done deliberately to ease reviews and debugging.

gentlementlegen commented 6 months ago

@pavlovcik Here is a fix with a sample preview of three different builds in the same comment: https://github.com/gentlementlegen/pay.ubq.fi/pull/13#issuecomment-2030089449 (will address the empty string as well just now)

gentlementlegen commented 6 months ago

@pavlovcik fixed the generate-permit.yml comments on the variable values and names. We can have the script in a file + faucet in another task.

QA results can be seen at https://github.com/gentlementlegen/pay.ubq.fi/pull/13#issuecomment-2030089449

github-actions[bot] commented 6 months ago
Preview Deployment
[941d46136a2d18f47a3bd70db94ddb3cc25e8941]()
0x4007 commented 6 months ago

Something I just realized but if its posting both deploy styles thats not preferred. We should clobber the old style with the new style in this repository. It also would be preferred to match the style because its intended to be easiest to match the shorthand commit that is present on the GitHub UI.

gentlementlegen commented 6 months ago

@pavlovcik Sure please let me know what you have in mind. I did it inside a table because on large screens for me it looks like

image

which is thought is a bit odd.

0x4007 commented 6 months ago

It is intended to be visually as close as possible to the current GitHub hash values. This is so that it's most efficient to match them when reviewing and testing. The only problem is that if we post new comments every time there is a significant amount of additional margin and padding which makes the pull request conversation length extremely long.