Skip to content

Faucet service & Iframe integration#661

Merged
GabrielMartinezRodriguez merged 16 commits into
masterfrom
gabriel/faucet
May 5, 2025
Merged

Faucet service & Iframe integration#661
GabrielMartinezRodriguez merged 16 commits into
masterfrom
gabriel/faucet

Conversation

@GabrielMartinezRodriguez

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

Linked Issues

Description

This PR introduces a new faucet service that allows users to receive tokens on HappyChain. Additionally, this service is integrated within the iframe.

Toggle Checklist

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Reminder: PR review guidelines

Correctness

  • C1. Builds and passes tests.
  • C2. The code is properly parameterized & compatible with different environments (e.g. local,
    testnet, mainnet, standalone wallet, ...).
  • C3. I have manually tested my changes & connected features.

< INDICATE BROWSER, DEMO APP & OTHER ENV DETAILS USED FOR TESTING HERE >

< INDICATE TESTED SCENARIOS (USER INTERFACE INTERACTION, CODE FLOWS) HERE >

  • C4. I have performed a thorough self-review of my code after submitting the PR,
    and have updated the code & comments accordingly.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code, by (1) using proper abstraction boundaries,
    (2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
  • D2. All public-facing APIs & meaningful (non-local) internal APIs are properly documented in code
    comments.
  • D3. If appropriate, the general architecture of the code is documented in a code comment or
    in a Markdown document.
  • D4. An appropriate Changeset has been generated (and committed) for changes that touch npm published packages (currently packages/core and packages/react), see here for more info.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Apr 22, 2025

Copy link
Copy Markdown

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2337b9f
Status: ✅  Deploy successful!
Preview URL: https://b2aef1c5.happychain.pages.dev
Branch Preview URL: https://gabriel-faucet.happychain.pages.dev

View logs

GabrielMartinezRodriguez commented Apr 22, 2025

Copy link
Copy Markdown
Contributor Author

@linear

linear Bot commented Apr 22, 2025

Copy link
Copy Markdown
HAPPY-436 Embed faucet in wallet

This would be a nice touch — will need syncing with Caldera considering the captcha system.

@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Apr 23, 2025
11 tasks
Comment thread apps/faucet/.env.example Outdated
Comment thread apps/faucet/.env.example Outdated
Comment thread apps/faucet/.env.example
Comment thread apps/faucet/Makefile

@norswap norswap Apr 24, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Viem seems to still be a peer deps of the common package, but wasn't actually used.

The issue is that because this is exported from the main index.ts, then it forces any consumer of common to have viem installed.

The fix is to have a separate lib/viem.ts that exports viem stuff, and then make a different subpath export in package.json — then people will import viem stuff via import { isAddress } from @happy.tech/common/viem and the main path will not require viem.

I was actually meaning to do that change, I want to move a new signTransaction function here that has our optimization for avoid the eth_getChainID call on walletClient.signTransaction.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread makefiles/vite.mk Outdated
Comment thread apps/faucet/src/errors.ts Outdated
Comment thread apps/faucet/src/utils.ts Outdated
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed updating Updating after review labels May 1, 2025
Comment thread apps/faucet/src/errors.ts Outdated

export class FaucetCaptchaError extends HappyFaucetError {
constructor(message?: string, options?: ErrorOptions) {
super(403, message || "Captcha verification failed", options)

@norswap norswap May 2, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
super(403, message || "Captcha verification failed", options)
super(403, message || "Captcha verification failed on the faucet service", options)

I ran into this and I thought the error layed with Cloudflare but it was with the service

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the service should await inclusion of the tx, so we can simply await response from the server on the wallet and invalidate the token balance query when the faucet completes. This will lead to a smooth flow: click > sending... > token sents, balance is updated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@norswap

norswap commented May 2, 2025

Copy link
Copy Markdown
Collaborator

The server is giving me a forbidden 403 (both before and after the changes I made. I didn't try to redeploy, maybe that would solve it?

Once this is resolved, let's merge this!

Main followup issue:

@norswap norswap added merge-after-changes Can be merged after requested changes are made and removed reviewing-2 Ready for, or undergoing final review labels May 2, 2025
@GabrielMartinezRodriguez

Copy link
Copy Markdown
Contributor Author

The server is giving me a forbidden 403 (both before and after the changes I made. I didn't try to redeploy, maybe that would solve it?

Once this is resolved, let's merge this!

Main followup issue:

Solved. The problem occurred because the deployed faucet didn't have the latest environment variables

@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit 3256439 into master May 5, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabriel/faucet branch May 5, 2025 10:32
@linear linear Bot mentioned this pull request May 5, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-after-changes Can be merged after requested changes are made priority-post-testnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants