fix(sentinel-api-service): gasCost is a decimal string on the wire, not a number - #10050
Open
gomesalexandre wants to merge 3 commits into
Open
fix(sentinel-api-service): gasCost is a decimal string on the wire, not a number#10050gomesalexandre wants to merge 3 commits into
gomesalexandre wants to merge 3 commits into
Conversation
…l string on the wire, not a number
The live Sentinel API (tx-sentinel-{network}.api.cx.metamask.io,
infura_simulateTransactions) emits gasCost as a quoted decimal string,
not a JSON number. Both TS declarations claimed number. Verified with
a live unauthenticated read-only simulation call - see PR body for the
raw response bytes.
SentinelSimulationResponseTransaction is public exported API, so a
downstream consumer typing against number and doing arithmetic on
gasCost gets silent string concatenation at runtime with no type
error. The one internal consumer (balance-changes.ts, new BN(offset))
happens to tolerate both a number and a base-10 string, which is why
this never surfaced as a visible bug.
…ted entry, add large-value test
gomesalexandre
marked this pull request as ready for review
September 1, 2026 13:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type-correctness fix, not a bugfix — nothing crashes today, but a public type was lying about the shape of the value it describes.
The mismatch
gasCostis declarednumberin two places:packages/sentinel-api-service/src/types.ts:455(SentinelSimulationResponseTransaction, publicly exported)packages/transaction-controller/src/api/simulation-api.ts:231(SimulationResponseTransaction, internal to the package)The live Sentinel API actually returns it as a quoted decimal string. Verified with a live, unauthenticated, read-only simulation call:
Response (trimmed):
feeEstimate(the adjacent field carrying the same kind of value) is a bare JSON number.gasCostis quoted. That contrast isn't an accident — the API deliberately string-encodes this specific field, almost certainly because the value can exceedNumber.MAX_SAFE_INTEGER(any transaction with a wei-scale gas cost ≥ ~0.009 ETH, trivially crossed on Polygon or at moderate mainnet gas prices).Why it's worth fixing
SentinelSimulationResponseTransactionis public exported API of@metamask/sentinel-api-service. A downstream consumer typing againstnumberand doing arithmetic (gasCost + 1) gets silent string concatenation at runtime ("51194859579161") with no type error, because TypeScript trusts the wrong declared type.The one internal consumer today,
getSimulationBalanceChangeinpackages/transaction-controller/src/utils/balance-changes.ts, passes the value straight tonew BN(offset), which happens to accept both a JS number and a base-10 string — which is exactly why this has never surfaced as a visible bug. The type was still wrong; nothing has hit the footgun yet.Every sibling numeric-ish field on the same type (
gas,gasLimit,gasUsed,maxFeePerGas,transferEstimate) is typedHex.gasCostbeingnumberis the lone outlier — a hand-written declaration that was never checked against a real response. Confirmed there are no recorded/VCR-style HTTP fixtures anywhere in the repo for this response shape; the only existing test fixtures are TS-typed builders (balance-changes.test.ts), so the suite has only ever exercised a wire shape the API doesn't actually emit.Fix
gasCost?: number→gasCost?: stringin both type declarations (+ corrected doc comments, "decimal number" → "decimal string").getSimulationBalanceChange'soffsetparameter:number = 0→string = '0'(matches hownew BNalready consumed it at runtime).createNativeBalanceResponse(balance-changes.test.ts) updated to build a string, matching the corrected type; one call site passing a numeric literal (2) updated to'2'.Number.MAX_SAFE_INTEGER(10500000000000000, realistic for Polygon), asserting it round-trips exactly throughBNwith no precision loss.Grepped the whole monorepo for any other reader of
gasCost(or theoffsetparam) doing numeric arithmetic, comparisons, or JSON round-tripping — found none. This is scoped to the two type declarations and their one real consumer.Scope note
Out of scope, flagged for awareness only: the real response also carries several fields undeclared in the TS type (
status,feeEstimate,baseFeePerGas,blockNumber,id,fees[].balanceNeeded/currentBalance). Left for a separate, narrower follow-up rather than scope-creeping this into a full response-shape audit.receipts
Ran a synchronous Codex adversarial review before opening. It confirmed the
BNsemantics are correct for the string path (including large values and the'0'default), found no missed consumers or stale numeric fixtures, and flagged two real issues which are both applied in this PR: thesentinel-api-servicechangelog entry needed a**BREAKING:**marker with migration guidance since the changed type is publicly exported (fixed), and thetransaction-controllerchangelog entry described a non-exported internal type with no consumer-facing behavior change and should be dropped per this repo's changelog conventions (removed).risk
Low for
transaction-controller(internal type, one consumer, behavior-preserving at runtime sincenew BNalready accepted strings). Thesentinel-api-servicechange is a breaking TS type change for any external consumer ofSentinelSimulationResponseTransaction.gasCost— called out with**BREAKING:**in that package's changelog with migration guidance.Note
Medium Risk
Breaking TypeScript contract for
@metamask/sentinel-api-serviceconsumers ofgasCost; runtime behavior in transaction-controller is largely unchanged becauseBNalready accepted strings.Overview
Aligns simulation response types with the Sentinel API:
gasCostis nowstring(wei as a decimal string) instead ofnumberonSentinelSimulationResponseTransactionand the internalSimulationResponseTransaction, with docs updated accordingly.@metamask/sentinel-api-servicedocuments this as a breaking public type change; consumers must parse withBN/BigIntbefore arithmetic.transaction-controllerpassesgasCostinto native balance math viagetSimulationBalanceChange, whose optionaloffsetis now a string defaulting to'0'(still fed tonew BN). Tests and fixtures use stringgasCost, including a case aboveNumber.MAX_SAFE_INTEGERso large wei values do not lose precision.Reviewed by Cursor Bugbot for commit 2a3c37e. Bugbot is set up for automated code reviews on this repo. Configure here.