Differential D20610

[cashtab] Require confirm for high AlpSwap price impact
ClosedPublic
Actions

Authored by bytesofman on Thu, Sep 10, 12:46.

Project Tags

None

Referenced Files

	F86023179: D20610.1791498336.diff
	Wed, Oct 7, 22:25

Subscribers

Fabien

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC8c6fe99567ea: [cashtab] Require confirm for high AlpSwap price impact

Summary

Quotes with 5% or greater price impact still settle, but Cashtab now
warns and asks the user to accept before broadcasting.

Test Plan

cd cashtab && npm test -- --testPathPatterns='AlpSwap/__tests__/index|alpSwapService' --watchAll=false

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bytesofman created this revision.Thu, Sep 10, 12:46

bytesofman held this revision as a draft.

Harbormaster completed remote builds in B38143: Diff 61313.Thu, Sep 10, 12:51

Build Bitcoin ABC Diffs / Diff Testing (ai-review) failed.

Tail of the build log:

/work /work/abc-ci-builds/ai-review
Build ai-review failed with exit code 1

Require a confirm for high price impact. Modal lists CoinGecko, then alp-dex spot and this swap, all in fiat per XEC.

bytesofman published this revision for review.Thu, Sep 10, 14:49

Harbormaster completed remote builds in B38157: Diff 61327.Thu, Sep 10, 14:58

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Your exception handling needs a therapist.

────────────────────────────────────────────────────────────────────────

minor [Functional Correctness]
→ ]8;;vscode://file//work/cashtab/src/components/AlpSwap/index.tsx:790cashtab/src/components/AlpSwap/index.tsx:790]8;;

Do not default an unknown priceImpactPct to zero.
SwapTemplateResponse.priceImpactPct is optional, and fetchSwapTemplate
does not validate or fill a missing value. ?? 0 therefore treats an
unknown impact as zero and can bypass the high-impact check. Preserve the
confirmation or hide the impact row when the value is absent.

────────────────────────────────────────
Review complete
1 finding ✔

Minor 1

5 files reviewed:

cashtab/src/components/AlpSwap/__tests__/index.test.tsx
cashtab/src/components/AlpSwap/index.tsx
cashtab/src/components/AlpSwap/styled.ts
cashtab/src/config/alpSwap.ts
cashtab/src/services/__tests__/alpSwapService.test.ts

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Fabien accepted this revision.Fri, Sep 11, 08:39

Fabien subscribed.

Fabien added inline comments.

cashtab/src/config/alpSwap.ts
73 ↗	(On Diff #61327)	Can we get a generic version of this? This is specific to the FIRMA/XECX pair but this can be useful for other pairs too. Maybe the lp should provide the price ?

This revision is now accepted and ready to land.Fri, Sep 11, 08:39

bytesofman marked an inline comment as done.Fri, Sep 11, 09:52

bytesofman added inline comments.

cashtab/src/config/alpSwap.ts
73 ↗	(On Diff #61327)	in this case it is a bit unique because Firma is a stablecoin; so the token<>token price implies a fiat price, and XECX (being 1:1 with XEC) also has a fiat price. For other token pairs though we won't have this, unless and until I add support for XEC AMMs. At that point though yes: we should also show the implied fiat price, and we could get it by using Cashtab's fiat price for whatever currency the user selected. e.g. the existing "GUNS<>BUTTER" pair cannot have a fiat price, since neither token trades against XEC and neither token trades against Firma.