Skip to content

Faucet: Limit usage to once per time window, increase quantity, and return the remaining wait time in case of a rate limit error.#727

Merged
GabrielMartinezRodriguez merged 3 commits intomasterfrom
gabruiel/faucet-increase-quantity
May 21, 2025
Merged

Faucet: Limit usage to once per time window, increase quantity, and return the remaining wait time in case of a rate limit error.#727
GabrielMartinezRodriguez merged 3 commits intomasterfrom
gabruiel/faucet-increase-quantity

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented May 6, 2025

Description

Faucet: Limit usage to once per time window, increase quantity, and return the remaining wait time in case of a rate limit error

  • Include all relevant context (but no need to repeat the issue's content).
  • Draw attention to new, noteworthy & unintuitive elements.
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 are documented in the docs.
    Public APIS and 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) with make changeset for
    breaking and meaningful changes in packages (not required for cleanups & refactors).

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 6, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: eea769e
Status:⚡️  Build in progress...

View logs

Copy link
Contributor Author

GabrielMartinezRodriguez commented May 6, 2025

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat(faucet): limt one tiem per window, increase quantity, and send pending time in case of rate limit error Limit one time per window, increase quantity, and send pending time in case of rate limit error May 6, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review May 6, 2025 14:04
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label May 6, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title Limit one time per window, increase quantity, and send pending time in case of rate limit error Faucet: Limit one time per time window, increase quantity, and send pending time in case of rate limit error May 6, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title Faucet: Limit one time per time window, increase quantity, and send pending time in case of rate limit error Faucet: Limit usage to once per time window, increase quantity, and return the remaining wait time in case of a rate limit error. May 6, 2025
@@ -1,4 +1,5 @@
import type { ContentfulStatusCode } from "hono/utils/http-status"
import ms from "ms"
Copy link
Contributor

Choose a reason for hiding this comment

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

cute little package! claude is telling me that this code will cover "most" cases though:

const unit = {
    ms: 1,
    s: 1e3,
    m: 60e3,
    h: 3_600e3,
    d: 86_400e3,
  } as const;
  
export function formatMs(ms: number, long = false): string {
    const abs = Math.abs(ms);
    for (const [abbr, size] of Object.entries(unit).reverse()) {
        if (abs >= size) {
        const val = Math.round(ms / size);
        return long ? `${val} ${abbr}${Math.abs(val) !== 1 ? 's' : ''}` : `${val}${abbr}`;
        }
    }
    return `${ms}ms`;
}

so im not sure if its worth the import? security risks (minimized by bun to be fair), 4kb package bloat (lol) to a backend service. not a hill i will die on

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 👍

return err(new FaucetRateLimitError())
if (faucetUsageResult.value.length >= 1) {
const lastRequest = faucetUsageResult.value[0]
const timeToWait =
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way this can be negative?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we might be missing an if(timeToWait>0)... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cannot be negative because, if it were, that would mean the request occurred before the time window, and we're only processing requests within the time window

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the reason is that the DB doesn't store uses longer than the window? This definitely could use a comment, and to be honest, a check wouldn't be a bad idea anyway just in case. e.g. if the service crashes, stays down for some time, then loads before the first prune can go through, the scenario of a negative number seems possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@aodhgan aodhgan added reviewing-2 Ready for, or undergoing final review question Something has to be cleared up after review and removed reviewing-1 Ready for, or undergoing first-line review labels May 8, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez removed the question Something has to be cleared up after review label May 12, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/faucet-wait-tx-inclusion branch from c7c4802 to bf2f73b Compare May 12, 2025 10:41
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabruiel/faucet-increase-quantity branch from 4109094 to efddb9b Compare May 12, 2025 10:41
@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 16, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/faucet-wait-tx-inclusion branch from bf2f73b to 7fef662 Compare May 21, 2025 08:56
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabruiel/faucet-increase-quantity branch from efddb9b to 7fff5cd Compare May 21, 2025 08:56
@GabrielMartinezRodriguez GabrielMartinezRodriguez added merge-blocked Ready to merge, waiting for downstack and removed merge-after-changes Can be merged after requested changes are made labels May 21, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/faucet-wait-tx-inclusion branch from 7fef662 to 0ad0f49 Compare May 21, 2025 09:12
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabruiel/faucet-increase-quantity branch from 7fff5cd to adc389f Compare May 21, 2025 09:12
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/faucet-wait-tx-inclusion branch from 0ad0f49 to 8a3cf6b Compare May 21, 2025 09:29
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabruiel/faucet-increase-quantity branch from adc389f to 85978f5 Compare May 21, 2025 09:29
Base automatically changed from gabriel/faucet-wait-tx-inclusion to master May 21, 2025 09:29
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabruiel/faucet-increase-quantity branch from 85978f5 to eea769e Compare May 21, 2025 09:30
@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit 387e134 into master May 21, 2025
1 of 3 checks passed
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabruiel/faucet-increase-quantity branch May 21, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-blocked Ready to merge, waiting for downstack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants