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

feat github middleware #41

Merged
merged 9 commits into from
Apr 11, 2025

Conversation

Villaquiranm
Copy link
Contributor

@Villaquiranm Villaquiranm commented Feb 22, 2025

Related to gnolang/gno#3808, this PR implements the GitHub middleware in the faucet hub.

Copy link

netlify bot commented Feb 22, 2025

Deploy Preview for gno-faucet-hub ready!

Name Link
🔨 Latest commit 0620456
🔍 Latest deploy log https://app.netlify.com/sites/gno-faucet-hub/deploys/67f924128e0ed600082bd484
😎 Deploy Preview https://deploy-preview-41--gno-faucet-hub.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

"name": "Teritori Faucet",
"chain_id": "test5",
"amounts": [1, 5, 10],
"url": "http://localhost:5050",
Copy link

Choose a reason for hiding this comment

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

Just to be sure: will this entry be removed or changed to a public hostname before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello :)
thanks for the review yes it is just for testing, I think we can leave the comment to change it before merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed here thanks for your review: 👍
2c849c3

@alexiscolin
Copy link
Member

alexiscolin commented Mar 17, 2025

I had a chat with @zxxma. I'm gonna take care of the UI side this week.

@alexiscolin
Copy link
Member

@Villaquiranm could you give me access to your fork so I can polish the front-end side in the PR? :)

@Villaquiranm
Copy link
Contributor Author

hello @alexiscolin yeah sure but It is a public repo, let me know what do you want me to do

@alexiscolin alexiscolin self-assigned this Mar 23, 2025
@alexiscolin alexiscolin marked this pull request as ready for review March 23, 2025 11:51
@alexiscolin
Copy link
Member

@Villaquiranm Should be better like that. However, we will need to setup the right info in the JSON as @aeddi mentioned.
@zivkovicmilos We will have to test that with prod data/security policies.

@alexiscolin
Copy link
Member

I've

  • rewritten the GH oauth method to follow the Captcha one style
  • pushed everything into the Store
  • gather all the animations into the store (to manage them in a easier way)
  • merged the GH oauth flow into the regular UI flow (even with the redirect)

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

We can go the extra step and improve the UI a bit more 🙏

The modal that has the GH authentication should also have clear visual cues that a GH authentication is needed, I've attached a mockup below. It's a subtle change, but really improves the UX, as I'm not randomly redirected to the GH application page when I click "Request drip"

@alexiscolin what do you think?

shapes at 25-04-01 20 35 44

@alexiscolin
Copy link
Member

We can go the extra step and improve the UI a bit more 🙏

The modal that has the GH authentication should also have clear visual cues that a GH authentication is needed, I've attached a mockup below. It's a subtle change, but really improves the UX, as I'm not randomly redirected to the GH application page when I click "Request drip"

@alexiscolin what do you think?

shapes at 25-04-01 20 35 44

@zivkovicmilos I totally agree that we should make it clearer that GitHub authentication is required. That’s a great UX improvement 🙏

However, I’d suggest avoiding having two separate buttons (one for GH auth and another for the request), since the current flow performs both actions at once. Displaying two buttons might give the user a false sense of choice, which doesn’t exist in the actual backend logic.

Instead, what about updating the single button to include a GitHub icon and clearer text, something like “Get Drip with GitHub” (or even shorter)? That way, it remains visually explicit and stays aligned with the actual flow.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

@alexiscolin
Works for me ✅

@zivkovicmilos
Copy link
Member

zivkovicmilos commented Apr 3, 2025

@Villaquiranm @alexiscolin
Let's just drop the Teritori faucet example, since the code is good to go

@Villaquiranm
Copy link
Contributor Author

@Villaquiranm @alexiscolin Let's just drop the Teritori faucet example, since the code is good to go

@zivkovicmilos @alexiscolin
thanks you both :)
I deployed a faucet and changed the url on the json file
everything seems to be working fine
2c849c3

zivkovicmilos pushed a commit to gnolang/gno that referenced this pull request Apr 11, 2025
related to #3781 
Related faucet-hub PR:
gnolang/faucet-hub#41

This pull request introduces two key features to gnofaucet:

**getGithubMiddleware**: A new middleware that checks for a code query
parameter in the URL. It attempts to exchange this code for a GitHub
token via OAuth. If the code is valid, the middleware retrieves the
GitHub login associated with the token.

**Cooldown Period**: This feature allows for a configurable cooldown
period (1 hour in this case). If the user attempts to claim tokens again
before the cooldown period expires, the middleware will reject the
request.

Additionally, we could enhance the functionality by implementing checks
for account age, pull requests, commits, or verifying if the user
belongs to a specific organization.

[screen-capture
(8).webm](https://github.com/user-attachments/assets/336d6767-9c7a-49eb-b4f8-7a514def628a)

---------

Co-authored-by: Your Name <you@example.com>
Co-authored-by: Antoine Eddi <5222525+aeddi@users.noreply.github.com>
@zivkovicmilos zivkovicmilos merged commit 6eb01ab into gnolang:main Apr 11, 2025
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.

4 participants