Skip to content

fix(predict): harden claim gas-station preflight - #35567

Draft
jpuri wants to merge 1 commit into
mainfrom
fix/predict-claim-gas-station-preflight
Draft

fix(predict): harden claim gas-station preflight#35567
jpuri wants to merge 1 commit into
mainfrom
fix/predict-claim-gas-station-preflight

Conversation

@jpuri

@jpuri jpuri commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

Predict claim can fail on Polygon Safe wallets when a pUSD gas fee token is pre-selected but Sentinel returns an empty gasFeeTokens list and native POL is insufficient. Users see a generic confirmation failure (Gas fee token not found and insufficient native balance / intrinsic gas too low: gas 0).

This PR hardens the mobile-side claim flow:

  1. PredictController.claimWithConfirmation — align batch submission with withdraw: gate gasFeeToken on wallet type (omit for deposit-wallet), and remove skipInitialGasEstimate so gas estimates complete before confirm.
  2. PolymarketProvider.beforeSignClaim — for Safe wallets, fail early with a clear error when simulation has finished and the selected gas fee token is unavailable.
  3. useIsGaslessLoading — treat an empty gasFeeTokens array as unavailable (not “done loading”) when native balance is insufficient, so confirm stays disabled.
  4. useInsufficientBalanceAlert — show the blocking insufficient-balance alert for forced gas-fee-token flows (isGasFeeTokenIgnoredIfBalance) when no gas-station tokens are available.

Companion core fix: MetaMask/core#10071 (transaction-controller gas fee token preflight). Mobile should bump @metamask/transaction-controller after core merges.

Changelog

CHANGELOG entry: Fixed Predict claim failures for users with insufficient POL when gas-station fee tokens are unavailable

Related issues

Refs: https://consensyssoftware.atlassian.net/browse/CONF-1725

Manual testing steps

Feature: Predict claim gas-station preflight

  Scenario: Safe wallet with insufficient POL and unavailable pUSD gas token
    Given a Polygon Predict Safe wallet with claimable positions
    And the EOA has low POL and insufficient pUSD for gas-station fees
    When the user opens Predict claim confirmation
    Then confirm remains disabled or a blocking insufficient-balance alert is shown
    And submitting does not produce a generic gas-station failure at sign time

  Scenario: Deposit-wallet claim
    Given a deposit-wallet Predict account with claimable positions
    When the user claims winnings
    Then the claim batch omits gasFeeToken and uses the relayer path

Screenshots/Recordings

N/A — preflight/confirmation gating change; manual verification on iOS/Android simulator recommended after core bump.

Before

N/A

After

N/A

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamask-ci metamask-ci Bot added the team-confirmations Push issues to confirmations team label Sep 2, 2026
@metamask-ci

metamask-ci Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Pre-merge author checklist has unchecked items (e.g. "I've applied the right labels on the PR (see labeling guidelines). Not required for external contributors."). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

Align claim batch submission with withdraw, validate Safe gas fee token
availability before sign, and block confirmation when gas-station tokens
are unavailable for insufficient native balance flows.
@jpuri
jpuri force-pushed the fix/predict-claim-gas-station-preflight branch from e695d65 to 4b31da9 Compare September 2, 2026 05:19
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokePredictions, SmokeConfirmations, SmokeWalletPlatform
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

E2E Test Selection:

PredictController.ts & PolymarketProvider.ts changes:

  • The PredictController now dynamically determines gasFeeToken based on account wallet type (deposit-wallet vs. other). For deposit-wallet accounts, gasFeeToken is undefined; for others, it uses MATIC collateral. This directly affects the claim flow in Polymarket predictions.
  • PolymarketProvider's beforeSign logic was refactored: the wallet type check was inverted, and error handling was added for unavailable gas fee tokens (throws 'Insufficient POL and pUSD' error). This affects claim signing behavior.
  • These changes require SmokePredictions to validate the claim flow works correctly for both wallet types.
  • Per SmokePredictions tag description: "When selecting SmokePredictions, also select SmokeWalletPlatform (Trending section) and SmokeConfirmations."

useInsufficientBalanceAlert.ts & useIsGaslessLoading.ts changes:

  • useInsufficientBalanceAlert adds a new condition for hasNoGasFeeTokenSelected using isGasFeeTokenIgnoredIfBalance flag - this affects when the insufficient balance alert is shown during confirmations.
  • useIsGaslessLoading refines gasless loading state with more granular pending/empty token checks - this affects the loading state shown during gasless transaction confirmations.
  • These hooks are used in the confirmation UI system broadly, not just for Predictions. SmokeConfirmations is needed to validate gasless flows and insufficient balance alert behavior.

No performance impact: These are logic/behavioral changes to prediction claim flows and confirmation hooks, not rendering or data-loading performance changes.

Performance Test Selection:
The changes are focused on business logic for gas fee token selection in Polymarket claims and confirmation hook refinements. There are no changes to rendering performance, data loading, app launch, or other performance-sensitive flows. No performance tests are warranted.

View GitHub Actions results

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/UI/Predict/providers/polymarket/PolymarketProvider.test.ts 0/194 0/286 0/430

AI-detected flaky patterns

app/components/UI/Predict/providers/polymarket/PolymarketProvider.test.ts

  • J9 — Module-level mutable let bindings not reset in beforeEach (high)
    • Throughout the file, many tests assign global.fetch = jest.fn().mockResolvedValue(...) directly. Because global.fetch is a mutable global binding and there is no afterEach that restores it (e.g. delete global.fetch or global.fetch = originalFetch), any test that sets global.fetch leaves it in place for subsequent tests. Tests that do NOT set global.fetch themselves — such as those in the describe('markets') block that rely on module-level mocks like mockFetchEventsFromPolymarketApi — may unexpectedly pick up a stale global.fetch mock from a prior test when Jest runs tests in a different order (e.g. with --randomize). Additionally, the outer beforeEach that calls jest.clearAllMocks() is declared after the describe('markets') and describe('getPrices') inner blocks in source order, which means those inner blocks do not benefit from the outer beforeEach cleanup. Adding an afterEach that deletes global.fetch eliminates the cross-test contamination.
    • Suggested fix in app/components/UI/Predict/providers/polymarket/PolymarketProvider.test.ts:1:
      -// Module-level (file scope) — no afterEach/beforeEach resets global.fetch
      -// global.fetch is assigned directly in many tests, e.g.:
      -global.fetch = jest.fn().mockResolvedValue({
      -  ok: true,
      -  json: jest.fn().mockResolvedValue({ events, next_cursor: null }),
      -});
      -// ... and in dozens of other tests throughout the file
      -// There is no afterEach(() => { delete global.fetch; }) or equivalent reset
      +// Add an afterEach at the top level of describe('PolymarketProvider') to restore global.fetch:
      +describe('PolymarketProvider', () => {
      +  const originalFetch = global.fetch;
      +
      +  afterEach(() => {
      +    global.fetch = originalFetch; // restore to pre-test state after every test
      +  });
      +
      +  // ... existing describe blocks and tests unchanged
      +});
  • J3 — Missing jest.clearAllMocks() / jest.resetAllMocks() (high)
    • The outer beforeEach that calls jest.clearAllMocks() and sets up default mock return values is declared in source order after the describe('markets'), describe('getPrices'), and describe('getCryptoTargetPrice') blocks. In Jest, beforeEach hooks are registered in the order they appear in source, but they apply to all tests in the enclosing describe regardless of source position — so the beforeEach does run for those inner blocks at runtime. However, the default mock implementations set in that beforeEach (e.g. mockComputeProxyAddress.mockReturnValue(legacySafeAddress), mockResolveDepositWalletAddress.mockResolvedValue(depositWalletAddress)) are NOT available to the describe('markets') tests because those tests run before the beforeEach is encountered in source order during the collection phase — wait, actually Jest collects all hooks first then runs them. The real risk is readability-driven bugs: a developer adding a new test inside describe('markets') may not realize the beforeEach exists and may not see that mockComputeProxyAddress is already set. More concretely, the describe('PolymarketProvider.subscribeToOrderbook') block at the bottom of the file has its own beforeEach that calls mockReset() on only three mocks, not jest.clearAllMocks(), meaning all other module-level mocks (e.g. mockAnalyticsTrackEvent, signer.signTypedMessage) carry call history from prior tests into the subscribeToOrderbook suite. Moving the outer beforeEach to the top of the describe block and ensuring describe('PolymarketProvider.subscribeToOrderbook') also calls jest.clearAllMocks() eliminates this risk.
    • Suggested fix in app/components/UI/Predict/providers/polymarket/PolymarketProvider.test.ts:1:
      -describe('PolymarketProvider', () => {
      -  const originalBuilderCode = process.env.MM_PREDICT_BUILDER_CODE;
      -  describe('markets', () => {
      -    it('returns keyset market result from feed events', async () => {
      -      // ...
      -    });
      -    // ... many more tests
      -  });
      -
      -  describe('getPrices', () => {
      -    // ... tests
      -  });
      -
      -  describe('getCryptoTargetPrice', () => {
      -    // ... tests
      -  });
      -
      -  beforeEach(() => {
      -    jest.clearAllMocks();
      -    mockComputeProxyAddress.mockReturnValue(legacySafeAddress);
      -    mockResolveDepositWalletAddress.mockResolvedValue(depositWalletAddress);
      -    // ...
      -  });
      +describe('PolymarketProvider', () => {
      +  const originalBuilderCode = process.env.MM_PREDICT_BUILDER_CODE;
      +
      +  // Move beforeEach to the TOP of the describe block so it is
      +  // visually and structurally clear it applies to all inner suites.
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +    mockComputeProxyAddress.mockReturnValue(legacySafeAddress);
      +    mockResolveDepositWalletAddress.mockResolvedValue(depositWalletAddress);
      +    mockCreateApiKey.mockResolvedValue({ /* ... */ });
      +    // ... rest of existing beforeEach setup
      +  });
      +
      +  describe('markets', () => { /* unchanged */ });
      +  describe('getPrices', () => { /* unchanged */ });
      +  // ...
      +});
      +
      +describe('PolymarketProvider.subscribeToOrderbook', () => {
      +  beforeEach(() => {
      +    jest.clearAllMocks(); // add this — replaces the three individual mockReset() calls
      +  });
      +  // ...
      +});

This check is informational only and does not block merging.

@sonarqubecloud

sonarqubecloud Bot commented Sep 2, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size-M team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant