Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: review
  • Loading branch information
xavier-brochard committed Aug 4, 2025
commit e892ffae1ce308c825c6cab23fec354b703bdd8b
2 changes: 1 addition & 1 deletion packages/snap/snap.manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"url": "https://github.com/MetaMask/snap-solana-wallet.git"
},
"source": {
"shasum": "aqOdyICdZWwwirkDtw8MwwMHNiqT/nJblgJCwYCyppw=",
"shasum": "k8Iw/hr3sRgRGK4eaVRyFUBwcXUL75cbze8Er3BlaSk=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const refreshConfirmationEstimation: OnCronjobHandler = async () => {
'[refreshConfirmationEstimation]',
);

// try {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshConfirmationEstimation}] Background event triggered`,
);
Expand Down Expand Up @@ -143,10 +142,4 @@ export const refreshConfirmationEstimation: OnCronjobHandler = async () => {
`[${ScheduleBackgroundEventMethod.RefreshConfirmationEstimation}] Could not update the interface. But rolled back status to fetched.`,
);
}
// } catch (error) {
// logger.warn(
// { error },
// `[${ScheduleBackgroundEventMethod.RefreshConfirmationEstimation}] Background event failed`,
// );
// }
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,88 +17,87 @@ import { ScheduleBackgroundEventMethod } from './ScheduleBackgroundEventMethod';
export const refreshSend: OnCronjobHandler = async () => {
const logger = createPrefixedLogger(baseLogger, '[refreshSend]');

try {
// try {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] Background event triggered`,
);

const [assets, mapInterfaceNameToId, preferences] = await Promise.all([
assetsService.getAll(),
state.getKey<UnencryptedStateValue['mapInterfaceNameToId']>(
'mapInterfaceNameToId',
),
getPreferences().catch(() => DEFAULT_SEND_CONTEXT.preferences),
]);

const assetTypes = assets.flatMap((asset) => asset.assetType);

const sendFormInterfaceId = mapInterfaceNameToId?.[SEND_FORM_INTERFACE_NAME];

// Don't do anything if the send form interface is not open
if (!sendFormInterfaceId) {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] Background event triggered`,
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ No send form interface found`,
);
return;
}

const [assets, mapInterfaceNameToId, preferences] = await Promise.all([
assetsService.getAll(),
state.getKey<UnencryptedStateValue['mapInterfaceNameToId']>(
'mapInterfaceNameToId',
),
getPreferences().catch(() => DEFAULT_SEND_CONTEXT.preferences),
]);

const assetTypes = assets.flatMap((asset) => asset.assetType);

const sendFormInterfaceId =
mapInterfaceNameToId?.[SEND_FORM_INTERFACE_NAME];

// Don't do anything if the send form interface is not open
if (!sendFormInterfaceId) {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ No send form interface found`,
);
return;
}

// Schedule the next run
await snap.request({
method: 'snap_scheduleBackgroundEvent',
params: { duration: 'PT30S', request: { method: 'refreshSend' } },
});

// First, fetch the token prices
const tokenPrices = await priceApiClient.getMultipleSpotPrices(
assetTypes,
preferences.currency,
);
// Schedule the next run
await snap.request({
method: 'snap_scheduleBackgroundEvent',
params: { duration: 'PT30S', request: { method: 'refreshSend' } },
});

// Save them in the state
await state.setKey('tokenPrices', tokenPrices);

// Get the current context
const interfaceContext =
await getInterfaceContextOrThrow<SendContext>(sendFormInterfaceId);

// We only want to refresh the token prices when the user is in the transaction confirmation stage
if (interfaceContext.stage !== 'transaction-confirmation') {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ Not in transaction confirmation stage`,
);
return;
}

if (!interfaceContext.assets) {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ No assets found`,
);
return;
}

// Update the current context with the new rates
const updatedInterfaceContext = {
...interfaceContext,
tokenPrices: {
...interfaceContext.tokenPrices,
...tokenPrices,
},
};

await updateInterface(
sendFormInterfaceId,
<Send context={updatedInterfaceContext} />,
updatedInterfaceContext,
);
// First, fetch the token prices
const tokenPrices = await priceApiClient.getMultipleSpotPrices(
assetTypes,
preferences.currency,
);

// Save them in the state
await state.setKey('tokenPrices', tokenPrices);
Copy link

Choose a reason for hiding this comment

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

Bug: Token Refresh Errors and Unnecessary Scheduling

The refreshSend background event has two issues:

  1. Missing Error Handling: It lacks error handling for token price fetching and state updates. Failures in these operations cause the event to crash silently, preventing token price updates.
  2. Unnecessary Scheduling: It unconditionally schedules its next run before checking if the send form interface is open. This results in unnecessary background events being scheduled even when the interface is closed, contrary to the intended behavior.
Fix in Cursor Fix in Web


// Get the current context
const interfaceContext =
await getInterfaceContextOrThrow<SendContext>(sendFormInterfaceId);

// We only want to refresh the token prices when the user is in the transaction confirmation stage
if (interfaceContext.stage !== 'transaction-confirmation') {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ✅ Background event suceeded`,
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ Not in transaction confirmation stage`,
);
} catch (error) {
logger.warn(
{ error },
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ Background event failed`,
return;
}

if (!interfaceContext.assets) {
logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ No assets found`,
);
return;
}

// Update the current context with the new rates
const updatedInterfaceContext = {
...interfaceContext,
tokenPrices: {
...interfaceContext.tokenPrices,
...tokenPrices,
},
};

await updateInterface(
sendFormInterfaceId,
<Send context={updatedInterfaceContext} />,
updatedInterfaceContext,
);

logger.info(
`[${ScheduleBackgroundEventMethod.RefreshSend}] ✅ Background event suceeded`,
);
// } catch (error) {
// logger.warn(
// { error },
// `[${ScheduleBackgroundEventMethod.RefreshSend}] ❌ Background event failed`,
// );
// }
};
Copy link

Choose a reason for hiding this comment

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

Bug

The refreshSend background event has two main issues related to its execution flow and error handling:

  1. Missing Error Handling: Critical operations, such as fetching token prices, saving them to state, and updating the interface, lack try-catch blocks. This differs from the previous cronjob version, which handled these failures gracefully.
  2. Incorrect Scheduling Order: The next background event is scheduled early in the handler, before checks for the send interface being open and before the token price fetching/state saving operations.

As a result:

  • If the send interface is closed, the event still schedules its next run before exiting, leading to an infinite loop of unnecessary background events.
  • If token price fetching or state saving fails, the current event throws an unhandled error, but the next event has already been scheduled, resulting in repeated, failed background event executions every 30 seconds, wasting resources.
Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Errors are already handled globally at the upper level, in src/index.ts. The try/catch was redundant.
  2. Wrong, we do schedule the next run after we've checked that the send form is open, which is the relevant condition that make sense to be checked.

Loading