Feebudgetguard#259
Conversation
…ctions with memo hashes
…onents, and enhance testing suite and CI/CD pipelines
|
@GiftedGiftB is attempting to deploy a commit to the ezedikeevan's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@GiftedGiftB Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
ezedike-evan
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The fee-budget logic in `solve.ts` and `route.ts` is the right direction, but there are several issues that need to be fixed before this can merge.
Blocking issues
1. PR title is not Conventional Commits — CI will fail
The title `Feebudgetguard` will be rejected by `.github/workflows/pr-title.yml`. It must follow the pattern `type(scope): description`, e.g.:
```
feat(router): add fee-budget guard with FeeBudgetExceededError (#122)
```
2. Accidental git submodule added
The diff contains:
```diff
+++ b/stellar-intel
@@ -0,0 +1 @@
+Subproject commit 838fdf3
```
This adds the repo as a submodule pointing at itself — a clear mistake. Please remove it: `git rm --cached stellar-intel && git commit`.
3. `lib/env.ts` indentation error
In `parseEnv()`, the new key is indented differently from every other key (two spaces instead of four):
```ts
// wrong
NEXT_PUBLIC_FEE_BUDGET_PCT: process.env.NEXT_PUBLIC_FEE_BUDGET_PCT,
```
Align it with the surrounding keys. This will cause a lint failure (`--max-warnings 0`).
4. `solve.ts` calls `env` at module load time — breaks in test environments
`FeeBudgetExceededError` constructor and `getMaxFeeForAmount` read `env.NEXT_PUBLIC_FEE_BUDGET_PCT` as a default parameter, meaning `parseEnv()` is called when the module is first imported. Any test that imports `solve.ts` without setting every required env var will throw immediately. The existing `tests/lib/router.solve.test.ts` works only because it passes an explicit `feeBudgetPct`. The error constructor still does:
```ts
constructor(amount: number, feeBudgetPct: number | undefined = env.NEXT_PUBLIC_FEE_BUDGET_PCT) {
```
This is evaluated at call time, but `env` is a module-level import that triggers `parseEnv()` on import. If the env schema doesn't validate (common in test/CI), the entire module fails. Use a lazy getter or pass the value explicitly instead.
5. New route `app/api/rates/route.ts` duplicates the existing `app/api/intent/offramp/route.ts`
There is already an anchor-rates API route at `app/api/intent/offramp/route.ts` on `main`. Adding a second rates endpoint at `/api/rates` without removing or rerouting the original creates a confusing split. This PR should either patch the existing route or provide a clear migration path — a new parallel route with no consumers is scope creep.
6. `tx-builder.ts` and its tests are out of scope for this issue
Issue #122 (fee-budget guard) estimated changes to `lib/router/solve.ts` and `lib/env.ts` only. The new `lib/stellar/tx-builder.ts` and `tests/tx-builder.spec.ts` are unrelated to fee budgeting — they belong in their own PR against the appropriate issue.
7. Checklist boxes unchecked with no justification
The PR template requires every box to be ticked or marked N/A with a reason. `typecheck`, `lint`, `build` are all left as "⏳ not run" with the blanket reason of "existing repo-wide ambient errors." That isn't a valid exemption — those issues need to be fixed or pre-existing failures isolated with `// @ts-expect-error` annotations. Please run all three and resolve new failures before requesting re-review.
How to fix
- Rename the PR title to follow Conventional Commits.
- `git rm --cached stellar-intel` and amend/commit.
- Fix the indentation in `lib/env.ts`.
- Remove `env` as a default-parameter dependency from `FeeBudgetExceededError` constructor and `getMaxFeeForAmount`; accept the value as a required param or read it at call site only.
- Remove `app/api/rates/route.ts` (or justify its relationship to the existing route).
- Move `lib/stellar/tx-builder.ts` and `tests/tx-builder.spec.ts` to a separate PR.
- Run `npm run typecheck && npm run lint && npm run build` and fix all new failures.
|
Thanks for the contribution, @GiftedGiftB! I'm closing this for now so it can come back in a cleaner form. Reasons:
Please reopen/resubmit a focused PR that links a single issue, contains only the fee-budget work, and is rebased on the current (now-green) |
closed #213
Summary
Added fee-budget enforcement for SEP-24 off-ramp quotes and fixed the related TypeScript compile issues in the new solver and env schema.
Linked issue
Closes none — this is a targeted maintenance/fix PR.
Changes
FeeBudgetExceededErrorfor typed route rejection.422 FeeBudgetExceededwhen no anchors meet budget.computeRateComparisonsignature and kept budget filtering outside core comparator.NEXT_PUBLIC_STELLAR_NETWORK.baseUrlfor path alias resolution.Testing notes
Automated
npm run typecheck· ⏳ not run due existing unrelated repo-wide ambient type errors in the current workspacenpm run lint· ⏳ not runnpm run test· ❌ not run full suite; targeted test run passednpm run build· ⏳ not runNew / modified tests
Manual verification
Screenshots / recordings
Checklist
Correctness
npm run typecheckpasses — ⏳ not run due unrelated repo-wide ambient type errorsnpm run lintpasses with zero new warnings — ⏳ not runnpm run testpasses; new behaviour has a testnpm run buildpasses — ⏳ not runData integrity
isMock,// MOCK,// TODO: replace with real data, or commented-out real codeAbortControllertimeout unchangedSecurity & non-custody
.env.localunchangedDocs
Release hygiene
Breaking changes
None.
For reviewers