diff --git a/packages/snap/src/config.ts b/packages/snap/src/config.ts index 0ad4f1e7..dd10d2a5 100644 --- a/packages/snap/src/config.ts +++ b/packages/snap/src/config.ts @@ -40,6 +40,16 @@ const parseIntegerStruct = ( (value: string) => (value === '' ? undefined : parseInt(value, 10)), ); +const parseFloatStruct = ( + minValue: number, + defaultValue: number, +): Struct => + coerce( + defaulted(min(number(), minValue), defaultValue), + string(), + (value: string) => (value === '' ? undefined : parseFloat(value)), + ); + /** * A struct for validating the network config. */ @@ -88,14 +98,14 @@ const ConfigStruct = object({ /** * The base fee multiplier for the Stellar network. */ - baseFeeMultiplier: parseIntegerStruct(1, 1.2), + baseFeeMultiplier: parseFloatStruct(1, 1.5), /** * The smart contract transaction fee multiplier for the Stellar network. * The multiplier is higher because smart contract transactions have tighter ledger limits than normal transactions. * * @see https://developers.stellar.org/docs/learn/fundamentals/fees-resource-limits-metering#ledger-limits */ - simulationFeeMultiplier: parseIntegerStruct(1, 1.5), + simulationFeeMultiplier: parseFloatStruct(1, 2), }), api: object({ tokenApi: object({ diff --git a/packages/snap/src/handlers/keyring/keyring.ts b/packages/snap/src/handlers/keyring/keyring.ts index 8e0c940b..f0fa3bbe 100644 --- a/packages/snap/src/handlers/keyring/keyring.ts +++ b/packages/snap/src/handlers/keyring/keyring.ts @@ -122,8 +122,18 @@ export class KeyringHandler implements Keyring { async handle(origin: string, request: JsonRpcRequest): Promise { const result = (await withCatchAndThrowSnapError(async () => { + this.#logger.debug('Handle keyring request', { + origin, + method: request.method, + }); validateOrigin(origin, request.method); - return handleKeyringRequest(this, request); + const keyringRequestResult = await handleKeyringRequest(this, request); + this.#logger.debug('Keyring request handled', { + origin, + method: request.method, + result: keyringRequestResult, + }); + return keyringRequestResult; }, this.#logger)) ?? null; return result; diff --git a/packages/snap/src/services/network/NetworkService.test.ts b/packages/snap/src/services/network/NetworkService.test.ts index 1bf64904..5690df17 100644 --- a/packages/snap/src/services/network/NetworkService.test.ts +++ b/packages/snap/src/services/network/NetworkService.test.ts @@ -193,12 +193,10 @@ describe('NetworkService', () => { await Promise.resolve(); const second = await networkService.getBaseFeeWithCache(scope); - expect(first).toStrictEqual( - new BigNumber(55).multipliedBy(AppConfig.transaction.baseFeeMultiplier), - ); - expect(second).toStrictEqual( - new BigNumber(55).multipliedBy(AppConfig.transaction.baseFeeMultiplier), - ); + // It doesn't equal to 55 * 1.5 = 82.5, + // because rounded up to 83. + expect(first).toStrictEqual(new BigNumber(83)); + expect(second).toStrictEqual(new BigNumber(83)); expect(fetchBaseFeeSpy).toHaveBeenCalledTimes(1); }); diff --git a/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.test.ts b/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.test.ts index 125df43d..4956cb90 100644 --- a/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.test.ts +++ b/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.test.ts @@ -330,7 +330,7 @@ describe('OnChainAccountSynchronizeService', () => { ); }); - it('keeps SEP-41 persisted with zero balance when new sync returns zero', async () => { + it('omits SEP-41 zero balance from balance payload while persisting it in state', async () => { setupTest(); const { @@ -373,9 +373,9 @@ describe('OnChainAccountSynchronizeService', () => { KeyringEvent.AccountBalancesUpdated, expect.objectContaining({ balances: { - [keyringAccount.id]: expect.objectContaining({ - [sep41Id]: { unit: 'USDC', amount: '0' }, - }), + [keyringAccount.id]: { + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, + }, }, }), ); @@ -394,7 +394,7 @@ describe('OnChainAccountSynchronizeService', () => { expect(sep41Row?.balance).toBe('0'); }); - it('still emits SEP-41 zero balance on later syncs after missed events', async () => { + it('omits SEP-41 zero balance from balance payload on later syncs after missed events', async () => { setupTest(); const { @@ -456,15 +456,18 @@ describe('OnChainAccountSynchronizeService', () => { expect(balanceEventCalls[3]?.[2]).toStrictEqual( expect.objectContaining({ balances: { - [keyringAccount.id]: expect.objectContaining({ - [sep41Id]: { unit: 'USDC', amount: '0' }, - }), + [keyringAccount.id]: { + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, + }, }, }), ); + expect(balanceEventCalls[3]?.[2]).not.toHaveProperty( + `balances.${keyringAccount.id}.${sep41Id}`, + ); }); - it('simulates client receiving sync 1 and 4 while missing 2 and 3 with trustline removal and SEP-41 zero', async () => { + it('replays asset-list add/remove on sync 4 while balance payload omits tombstones and zero SEP-41', async () => { setupTest(); const { @@ -599,19 +602,28 @@ describe('OnChainAccountSynchronizeService', () => { isKeyringEmitCall(call, KeyringEvent.AccountBalancesUpdated), ) .map((call) => call[2]); + const assetListEventCalls = emitSnapKeyringEventSpy.mock.calls + .filter((call) => + isKeyringEmitCall(call, KeyringEvent.AccountAssetListUpdated), + ) + .map((call) => call[2]); expect(balanceEventCalls).toHaveLength(4); - const sync1Payload = balanceEventCalls[0] as { + expect(assetListEventCalls).toHaveLength(4); + const sync1BalancePayload = balanceEventCalls[0] as { balances: Record< string, Record >; }; - const sync4Payload = balanceEventCalls[3] as { + const sync4BalancePayload = balanceEventCalls[3] as { balances: Record< string, Record >; }; + const sync4AssetListPayload = assetListEventCalls[3] as { + assets: Record; + }; const simulatedClientBalances: Record< string, @@ -621,7 +633,7 @@ describe('OnChainAccountSynchronizeService', () => { // Client receives sync 1. Object.assign( simulatedClientBalances, - sync1Payload.balances[keyringAccount.id] as Record< + sync1BalancePayload.balances[keyringAccount.id] as Record< string, { amount: string; unit: string } >, @@ -631,26 +643,32 @@ describe('OnChainAccountSynchronizeService', () => { // Client misses sync 2 and 3. - // Client receives sync 4 and catches up. + // Client receives sync 4 balance patch: visible assets only; stale keys from sync 1 remain. Object.assign( simulatedClientBalances, - sync4Payload.balances[keyringAccount.id] as Record< + sync4BalancePayload.balances[keyringAccount.id] as Record< string, { amount: string; unit: string } >, ); - // Trustline removal happened on sync 2 and was missed by client. - // Classic removals persist as tombstone entries (`limit` 0), so sync 4 still sends balance 0. - expect(simulatedClientBalances[USDC_CLASSIC]?.amount).toBe('0'); - // SEP-41 zero entries are persisted and continue to be sent by sync balance update events, - // so sync 4 corrects the client if it missed a previous update. - expect(simulatedClientBalances[sep41Id]).toStrictEqual({ - unit: 'USDC', - amount: '0', + expect(simulatedClientBalances[USDC_CLASSIC]?.amount).toBe('25'); + expect(simulatedClientBalances[sep41Id]?.amount).toBe('0.00005'); + expect(simulatedClientBalances[NATIVE]?.amount).toBe('1'); + + // Asset list replay on sync 4 reconciles visibility the client missed. + expect(sync4AssetListPayload.assets[keyringAccount.id]).toStrictEqual({ + added: expect.arrayContaining([NATIVE]), + removed: expect.arrayContaining([USDC_CLASSIC, sep41Id]), }); + expect(sync4BalancePayload.balances[keyringAccount.id]).not.toHaveProperty( + USDC_CLASSIC, + ); + expect(sync4BalancePayload.balances[keyringAccount.id]).not.toHaveProperty( + sep41Id, + ); }); - it('runs four syncs with emit failures then full balance and asset-list reconciliation', async () => { + it('runs four syncs with emit failures then reconciles via replayed asset-list and visible balances', async () => { setupTest(); const { @@ -810,34 +828,37 @@ describe('OnChainAccountSynchronizeService', () => { return row!; }; - // 1st `AccountBalancesUpdated` (after sync 1): USDC trustline at 0 + full snapshot. + // 1st `AccountBalancesUpdated` (after sync 1): USDC trustline at 0 + native. expect(accountBalancesFromNthBalanceEmit(0)).toMatchObject({ + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, [USDC_CLASSIC]: { unit: 'USDC', amount: '0' }, }); - // 2nd `AccountBalancesUpdated` (after sync 2): USDC removed on Horizon → tombstone balance 0 in payload. - expect(accountBalancesFromNthBalanceEmit(1)).toMatchObject({ - [USDC_CLASSIC]: { unit: 'USDC', amount: '0' }, + // 2nd `AccountBalancesUpdated` (after sync 2): USDC tombstone omitted; native only. + expect(accountBalancesFromNthBalanceEmit(1)).toStrictEqual({ + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, }); - // 3rd `AccountBalancesUpdated` (after sync 3): EURC added on Horizon + USDC tombstone still in snapshot. + // 3rd `AccountBalancesUpdated` (after sync 3): EURC visible; USDC tombstone omitted. expect(accountBalancesFromNthBalanceEmit(2)).toMatchObject({ - [USDC_CLASSIC]: { unit: 'USDC', amount: '0' }, + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, [EURC_CLASSIC]: { unit: 'EURC', amount: '10' }, }); + expect(accountBalancesFromNthBalanceEmit(2)).not.toHaveProperty( + USDC_CLASSIC, + ); - // 4th `AccountBalancesUpdated` (after sync 4): same on-chain entries as sync 3, but `find` read a stale - // baseline so asset-list deltas fire; balances stay a full per-asset map (native + SEP-41 + classics). + // 4th `AccountBalancesUpdated` (after sync 4): same visible balances as sync 3. const fourthBalanceSnapshot = accountBalancesFromNthBalanceEmit(3); - expect(Object.keys(fourthBalanceSnapshot).length).toBeGreaterThan(2); - expect(fourthBalanceSnapshot).toMatchObject({ - [USDC_CLASSIC]: { unit: 'USDC', amount: '0' }, + expect(fourthBalanceSnapshot).toStrictEqual({ + [NATIVE]: { unit: NATIVE_ASSET_SYMBOL, amount: '1' }, [EURC_CLASSIC]: { unit: 'EURC', amount: '10' }, }); const assetListCalls = emitSnapKeyringEventSpy.mock.calls.filter((call) => isKeyringEmitCall(call, KeyringEvent.AccountAssetListUpdated), ); + // Asset-list emit runs after balance emit in the same try block; sync 2 and 3 balance failures skip it. expect(assetListCalls).toHaveLength(2); const assetListDeltaFromNthAssetEmit = ( @@ -857,13 +878,13 @@ describe('OnChainAccountSynchronizeService', () => { // 1st `AccountAssetListUpdated` (after sync 1): USDC trustline becomes visible (limit > 0, balance 0). expect(assetListDeltaFromNthAssetEmit(0)).toStrictEqual({ added: expect.arrayContaining([NATIVE, USDC_CLASSIC]), - removed: [], + removed: expect.arrayContaining([sep41Id]), }); - // 2nd `AccountAssetListUpdated` (after sync 4): stale baseline vs current → EURC added, USDC removed from list. + // 2nd `AccountAssetListUpdated` (after sync 4): replays on-chain visibility — EURC added, USDC removed. expect(assetListDeltaFromNthAssetEmit(1)).toStrictEqual({ added: expect.arrayContaining([NATIVE, EURC_CLASSIC]), - removed: [USDC_CLASSIC], + removed: expect.arrayContaining([USDC_CLASSIC, sep41Id]), }); }); diff --git a/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.ts b/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.ts index ab87abb0..f5f140fa 100644 --- a/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.ts +++ b/packages/snap/src/services/on-chain-account/OnChainAccountSynchronizeService.ts @@ -432,7 +432,7 @@ export class OnChainAccountSynchronizeService { * * @param stateSnapshotOnChainAccount - Last saved account from state, or `null` when none exists. * @param synchronizedOnChainAccount - Same account after Horizon, SEP-41, and merge steps. - * @returns `balanceChanges` for all known assets plus optional `assetListChanges`. + * @returns `balanceChanges` for on-chain-visible assets plus `assetListChanges` replayed each sync. */ #computeKeyringSyncDeltas( stateSnapshotOnChainAccount: OnChainAccount | null, @@ -458,53 +458,63 @@ export class OnChainAccountSynchronizeService { : stateSnapshotOnChainAccount.getRawAsset(assetId); const onChainEntry = synchronizedOnChainAccount.getRawAsset(assetId); - // Always send the full balance snapshot, even when values did not change. - // This lets the client recover if it missed a previous balances event. - // Example with 4 assets: - // - XLM (native), USDC classic trustline, EURC classic trustline, SOLBTC SEP-41. + // assetIds = union of state + on-chain rawAssetIds (includes tombstones and zero SEP-41). + // Asset list replays every sync from on-chain visibility so a client can recover missed events: + // visible on-chain → added; not visible on-chain (but still in assetIds) → removed. + // Balances are emitted only for on-chain-visible assets. + // Example: XLM (native), USDC/EURC/AQUA (classic trustlines), SOLBTC (SEP-41). // ------------------------- Sync 1 ------------------------------------------ // - Sync 1 (success): payload received by client: - // XLM=10 (raw total), USDC=25, EURC=0, SOLBTC=5. + // state view: none (new account). + // onChain view: XLM=10, USDC=25, EURC=0, SOLBTC=5. + // balance payload: XLM=10, USDC=25, EURC=0, SOLBTC=5. + // asset list payload: added XLM, USDC, EURC, SOLBTC; removed none. // ------------------------- Sync 2 ------------------------------------------ - // - Sync 2 (client misses event): chain updates to - // XLM=11, USDC=30, EURC trustline removed (it was already zero), SOLBTC=0. - // Payload for this sync would include EURC amount=0 and SOLBTC amount=0, - // but client missed it. + // - Sync 2 (client misses event): + // state view: XLM=10, USDC=25, EURC=0, SOLBTC=5. + // onChain view: XLM=11, USDC=30, AQUA trustline added, EURC trustline removed, SOLBTC=0. + // balance payload: XLM=11, USDC=30, AQUA=0. + // asset list payload: added XLM, USDC, AQUA; removed EURC, SOLBTC. // ------------------------- Sync 3 ------------------------------------------ - // - Sync 3 (client misses event): chain updates to - // XLM=9, USDC=30, SOLBTC still 0. - // Because SEP-41 zero balances are persisted, payload still includes SOLBTC=0. + // - Sync 3 (client misses event): + // state view: XLM=11, USDC=30, AQUA trustline, EURC tombstone, SOLBTC=0. + // onChain view: XLM=9, USDC=30, AQUA trustline, EURC tombstone, SOLBTC=0. + // balance payload: XLM=9, USDC=30, AQUA. + // asset list payload: added XLM, USDC, AQUA; removed EURC, SOLBTC (replayed). // ------------------------- Sync 4 ------------------------------------------ - // - Sync 4 (success): we still emit full balances for on-chain view + latest snapshot, - // so payload includes XLM=9 (raw total), USDC=30, SOLBTC=0. - // Classic trustlines removed on chain are persisted as internal tombstones (`limit` 0), - // so sync 4 can still send balance `0` for those asset ids if the client missed earlier events. - balanceChanges[assetId as string] = - assetId === nativeAssetId - ? { - unit: NATIVE_ASSET_SYMBOL, - amount: toDisplayBalance( - synchronizedOnChainAccount.nativeRawBalance, - ), - } - : this.#buildBalancePayloadFromEntries( - onChainEntry, - latestStateEntry, - ); - - const isVisibleFromState = this.#isAssetVisible( - assetId, - latestStateEntry, - ); + // - Sync 4 (success): payload received by client: + // state view: XLM=9, USDC=30, AQUA trustline, EURC tombstone, SOLBTC=0. + // onChain view: XLM=9, USDC=30, AQUA trustline, EURC tombstone, SOLBTC=0. + // balance payload: XLM=9, USDC=30, AQUA. + // asset list payload: added XLM, USDC, AQUA; removed EURC, SOLBTC (replayed). const isVisibleFromOnChain = this.#isAssetVisible(assetId, onChainEntry); - // Add/remove is based on visibility transition between snapshots. - if (isVisibleFromOnChain && !isVisibleFromState) { + + // Asset list: replay add/remove from on-chain visibility (not transition-based). + if (isVisibleFromOnChain) { addedAssets.push(assetId); } - if (isVisibleFromState && !isVisibleFromOnChain) { + if (!isVisibleFromOnChain) { removedAssets.push(assetId); } + + // Balance: on-chain-visible assets only (tombstones and zero SEP-41 omitted). + // There is no need to emit balance for assets that are not visible from on-chain. + // The client controller will remove the balance entry if they receive a asset list event with the asset removed. + if (isVisibleFromOnChain) { + balanceChanges[assetId as string] = + assetId === nativeAssetId + ? { + unit: NATIVE_ASSET_SYMBOL, + amount: toDisplayBalance( + synchronizedOnChainAccount.nativeRawBalance, + ), + } + : this.#buildBalancePayloadFromEntries( + onChainEntry, + latestStateEntry, + ); + } } // Native is always persisted on activated accounts; diff --git a/packages/snap/src/services/transaction/KeyringTransactionBuilder.test.ts b/packages/snap/src/services/transaction/KeyringTransactionBuilder.test.ts index 4fb36a13..d952be94 100644 --- a/packages/snap/src/services/transaction/KeyringTransactionBuilder.test.ts +++ b/packages/snap/src/services/transaction/KeyringTransactionBuilder.test.ts @@ -106,9 +106,12 @@ describe('KeyringTransactionBuilder', () => { }, }); - expect(transaction.type).toBe(TransactionType.Unknown); + expect(transaction.type).toBe(TransactionType.TokenApprove); expect(transaction.id).toBe('tx-opt-in-1'); expect(transaction.status).toBe(TransactionStatus.Unconfirmed); + expect(transaction.details).toStrictEqual({ + typeLabel: 'trustline-approve', + }); expect(transaction.events).toStrictEqual([ { status: TransactionStatus.Unconfirmed, timestamp: fixedTimestamp }, ]); @@ -155,8 +158,11 @@ describe('KeyringTransactionBuilder', () => { }, }); - expect(transaction.type).toBe(TransactionType.Unknown); + expect(transaction.type).toBe(TransactionType.TokenDisapprove); expect(transaction.status).toBe(TransactionStatus.Confirmed); + expect(transaction.details).toStrictEqual({ + typeLabel: 'trustline-disapprove', + }); expect(transaction.events).toStrictEqual([ { status: TransactionStatus.Confirmed, timestamp: fixedTimestamp }, ]); diff --git a/packages/snap/src/services/transaction/KeyringTransactionBuilder.ts b/packages/snap/src/services/transaction/KeyringTransactionBuilder.ts index 93285dd9..1f92bf74 100644 --- a/packages/snap/src/services/transaction/KeyringTransactionBuilder.ts +++ b/packages/snap/src/services/transaction/KeyringTransactionBuilder.ts @@ -19,6 +19,11 @@ export enum KeyringTransactionType { Unknown = 'unknown', } +enum KeyringTransactionTypeLabel { + ChangeTrustOptIn = 'trustline-approve', + ChangeTrustOptOut = 'trustline-disapprove', +} + export type KeyringTransactionAsset = { unit: string; type: KnownCaip19AssetIdOrSlip44Id; @@ -126,7 +131,10 @@ export class KeyringTransactionBuilder { switch (request.type) { case KeyringTransactionType.ChangeTrustOptOut: case KeyringTransactionType.ChangeTrustOptIn: - return this.#createChangeTrustTransaction(request.request); + return this.#createChangeTrustTransaction( + request.request, + request.type, + ); case KeyringTransactionType.Send: return this.#createSendTransaction(request.request); case KeyringTransactionType.Swap: @@ -143,8 +151,23 @@ export class KeyringTransactionBuilder { } } + /** + * Builds a change-trust (trustline) keyring transaction. + * + * Maps opt-in to {@link TransactionType.TokenApprove} and opt-out to + * {@link TransactionType.TokenDisapprove}. Sets `details.typeLabel` so the UI can + * distinguish trustline approve vs disapprove until keyring types are fully adopted. + * + * @param request - Change-trust transaction fields (asset, fees, status, etc.). + * @param optInOrOut - Whether this is a trustline opt-in or opt-out. + * @returns A keyring transaction with token approve/disapprove type and UI type label. + * @see {@link https://github.com/MetaMask/accounts/pull/568} + */ #createChangeTrustTransaction( request: ChangeTrustTransactionRequest, + optInOrOut: + | KeyringTransactionType.ChangeTrustOptIn + | KeyringTransactionType.ChangeTrustOptOut, ): KeyringTransaction { const { txId, @@ -154,10 +177,18 @@ export class KeyringTransactionBuilder { status = TransactionStatus.Unconfirmed, } = request; const timestamp = this.#resolveTimestamp(request.timestamp); + const type = + optInOrOut === KeyringTransactionType.ChangeTrustOptIn + ? TransactionType.TokenApprove + : TransactionType.TokenDisapprove; + + const typeLabel = + optInOrOut === KeyringTransactionType.ChangeTrustOptIn + ? KeyringTransactionTypeLabel.ChangeTrustOptIn + : KeyringTransactionTypeLabel.ChangeTrustOptOut; return this.#buildKeyringTransaction({ - // TODO: Add the correct type - type: TransactionType.Unknown, + type, id: txId, account, scope, @@ -166,6 +197,9 @@ export class KeyringTransactionBuilder { status, timestamp, fees: request.fees ?? [], + details: { + typeLabel, + }, }); } @@ -318,6 +352,7 @@ export class KeyringTransactionBuilder { status, timestamp, fees, + details, }: { type: TransactionType; id: string; @@ -328,6 +363,7 @@ export class KeyringTransactionBuilder { status: TransactionStatus; timestamp: number; fees: KeyringTransaction['fees']; + details?: KeyringTransaction['details']; }): KeyringTransaction { return { type, @@ -345,6 +381,7 @@ export class KeyringTransactionBuilder { account: account.id, timestamp, fees, + ...(details === undefined ? {} : { details }), }; } diff --git a/packages/snap/src/services/transaction/TransactionMapper.test.ts b/packages/snap/src/services/transaction/TransactionMapper.test.ts index 1cee0c74..65c34452 100644 --- a/packages/snap/src/services/transaction/TransactionMapper.test.ts +++ b/packages/snap/src/services/transaction/TransactionMapper.test.ts @@ -231,7 +231,10 @@ describe('TransactionMapper', () => { toAmount: '0', toAddress: accountAddress, fromAddress: accountAddress, - txnType: TransactionType.Unknown, + txnType: TransactionType.TokenApprove, + details: { + typeLabel: 'trustline-approve', + }, }, { testCase: 'remove change trust transaction', @@ -252,7 +255,10 @@ describe('TransactionMapper', () => { toAmount: '0', toAddress: accountAddress, fromAddress: accountAddress, - txnType: TransactionType.Unknown, + txnType: TransactionType.TokenDisapprove, + details: { + typeLabel: 'trustline-disapprove', + }, }, { testCase: 'send transaction', @@ -362,6 +368,7 @@ describe('TransactionMapper', () => { txnType, fromAddress, toAddress, + details, }) => { const { keyringAccount, transactionMapper } = setup(); @@ -421,6 +428,8 @@ describe('TransactionMapper', () => { }, }, ], + // eslint-disable-next-line jest/no-conditional-in-test + ...(details ? { details } : {}), }); }, );