diff --git a/.changeset/proud-otters-joke.md b/.changeset/proud-otters-joke.md new file mode 100644 index 00000000..9bef55b5 --- /dev/null +++ b/.changeset/proud-otters-joke.md @@ -0,0 +1,5 @@ +--- +'guard-service': minor +--- + +refactor cold storage tx verification and its insertion handler to support parallel cold storage txs diff --git a/src/db/DatabaseHandler.ts b/src/db/DatabaseHandler.ts index 6d8e18e2..59657519 100644 --- a/src/db/DatabaseHandler.ts +++ b/src/db/DatabaseHandler.ts @@ -121,35 +121,11 @@ class DatabaseHandler { await DatabaseAction.getInstance().getActiveColdStorageTxsInChain( newTx.network ); - if (txs.length > 1) { - throw new ImpossibleBehavior( - `Chain [${newTx.network}] has already more than 1 (${txs.length}) active cold storage tx` + if (txs.some((tx) => tx.txId === newTx.txId)) { + logger.info( + `Reinsertion for cold storage tx [${newTx.txId}], 'failedInSign' updated to false` ); - } else if (txs.length === 1) { - const tx = txs[0]; - if (tx.txId === newTx.txId) { - logger.info( - `Reinsertion for cold storage tx [${tx.txId}], 'failedInSign' updated to false` - ); - await DatabaseAction.getInstance().resetFailedInSign(tx.txId); - } else { - if (tx.status === TransactionStatus.approved) { - if (newTx.txId < tx.txId) { - logger.info( - `Replacing cold storage tx [${tx.txId}] with new transaction [${newTx.txId}] due to lower txId` - ); - await DatabaseAction.getInstance().replaceTx(tx.txId, newTx); - } else - logger.info( - `Ignoring new cold storage tx [${newTx.txId}] due to higher txId, comparing to [${tx.txId}]` - ); - } else { - if (newTx.txId !== tx.txId) - logger.warn( - `Received approval for new tx [${newTx.txId}] where its chain [${newTx.network}] has already in progress tx [${tx.txId}]` - ); - } - } + await DatabaseAction.getInstance().resetFailedInSign(newTx.txId); } else await DatabaseAction.getInstance().insertNewTx(newTx, null, requiredSign); }; diff --git a/src/verification/RequestVerifier.ts b/src/verification/RequestVerifier.ts index 629661f1..84ba5988 100644 --- a/src/verification/RequestVerifier.ts +++ b/src/verification/RequestVerifier.ts @@ -90,7 +90,6 @@ class RequestVerifier { /** * verifies the cold storage transaction request sent by other guards * conditions: - * - there is no active cold storage transaction * - requested tx is not malicious * @param tx the created payment transaction * @returns true if conditions are met @@ -100,22 +99,6 @@ class RequestVerifier { ): Promise => { const baseError = `Received cold storage tx [${tx.txId}] `; - // check if event has any active tx for requested tx type - const inProgressColdStorageTxs = - await DatabaseAction.getInstance().getActiveColdStorageTxsInChain( - tx.network - ); - if ( - inProgressColdStorageTxs.length !== 0 && - inProgressColdStorageTxs[0].txId !== tx.txId - ) { - logger.warn( - baseError + - `but found active cold storage tx [${inProgressColdStorageTxs[0].txId}]` - ); - return false; - } - // verify requested tx if (!(await TransactionVerifier.verifyColdStorageTransaction(tx))) { logger.warn(baseError + `but tx hasn't verified`); diff --git a/src/verification/TransactionVerifier.ts b/src/verification/TransactionVerifier.ts index 303f4165..a9c8ffb5 100644 --- a/src/verification/TransactionVerifier.ts +++ b/src/verification/TransactionVerifier.ts @@ -16,6 +16,7 @@ import { JsonBI } from '../network/NetworkModels'; import { DatabaseAction } from '../db/DatabaseAction'; import WinstonLogger from '@rosen-bridge/winston-logger'; import { ChainNativeToken } from '../utils/constants'; +import * as TransactionSerializer from '../transaction/TransactionSerializer'; const logger = WinstonLogger.getInstance().getLogger(import.meta.url); @@ -117,9 +118,10 @@ class TransactionVerifier { * verifies the cold storage transaction * conditions: * - tx order is to cold storage address - * - at least one asset required transfer + * - at least one asset requires transfer * - no forbidden asset is transferred - * - address remaining assets are more than low threshold + * - no active cold storage tx exist that is transferring the same token + * - transferring assets remain more than low threshold in the lock address * @param tx the created payment transaction * @returns true if conditions are met */ @@ -159,13 +161,47 @@ class TransactionVerifier { const nativeTokenId = ChainNativeToken[tx.network]; const isNativeTokenForbade = forbiddenTokens.includes(nativeTokenId); + // no active cold storage tx exist that is transferring the same token + const thresholdsConfig = Configs.thresholds()[tx.network]; + const transferringTokenIds = txOrder[0].assets.tokens.map( + (token) => token.id + ); + const inProgressColdStorageTxs = + await DatabaseAction.getInstance().getActiveColdStorageTxsInChain( + tx.network + ); + for (const activeTx of inProgressColdStorageTxs) { + if (activeTx.txId === tx.txId) continue; + const activeTxOrder = chain.extractTransactionOrder( + TransactionSerializer.fromJson(activeTx.txJson) + ); + if ( + activeTxOrder[0].assets.tokens.some((token) => + transferringTokenIds.includes(token.id) + ) + ) { + logger.debug( + `Transaction [${tx.txId}] is invalid: Tx is transferring a token that is in transfer by tx [${activeTx.txId}]` + ); + return false; + } + if ( + txOrder[0].assets.nativeToken > thresholdsConfig.maxNativeTransfer && + activeTxOrder[0].assets.nativeToken > thresholdsConfig.maxNativeTransfer + ) { + logger.debug( + `Transaction [${tx.txId}] is invalid: Tx is transferring native token while it is also in transfer by tx [${activeTx.txId}]` + ); + return false; + } + } + // verify address assets const lockedAssets = await chain.getLockAddressAssets(); const remainingAssets = ChainUtils.subtractAssetBalance( lockedAssets, txOrder[0].assets ); - const thresholdsConfig = Configs.thresholds()[tx.network]; const thresholds = thresholdsConfig.tokens; // verify transfer conditions for tokens diff --git a/tests/db/DatabaseHandler.spec.ts b/tests/db/DatabaseHandler.spec.ts index d92b7451..0ca894d7 100644 --- a/tests/db/DatabaseHandler.spec.ts +++ b/tests/db/DatabaseHandler.spec.ts @@ -300,7 +300,7 @@ describe('DatabaseHandler', () => { describe('insertColdStorageTx', () => { /** * @target DatabaseHandler.insertColdStorageTx should insert tx when - * there is no other tx for the chain + * tx is not in database * @dependencies * - database * @scenario @@ -310,7 +310,7 @@ describe('DatabaseHandler', () => { * @expected * - tx should be inserted into db */ - it('should insert tx when there is no other tx for the chain', async () => { + it('should insert tx when tx is not in database', async () => { // mock transaction const chain = 'chain'; const tx = TxTestData.mockPaymentTransaction( @@ -331,193 +331,6 @@ describe('DatabaseHandler', () => { expect(dbTxs).toEqual([[tx.txId, null, tx.network]]); }); - /** - * @target DatabaseHandler.insertColdStorageTx should insert tx when - * there is invalid tx for the chain - * @dependencies - * - database - * @scenario - * - mock transaction - * - run test (call `insertTx`) - * - check database - * @expected - * - tx should be inserted into db - */ - it('should insert tx when there is invalid tx for the chain', async () => { - // mock transaction - const chain = 'chain'; - const tx = TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - const tx2 = TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - - // insert one of the txs into db - await DatabaseHandlerMock.insertTxRecord(tx2, TransactionStatus.invalid); - - // run test - await DatabaseHandler.insertTx(tx, requiredSign); - - // tx should be inserted into db - const dbTxs = (await DatabaseHandlerMock.allTxRecords()) - .filter((tx) => tx.txId !== tx2.txId) - .map((tx) => [tx.txId, tx.event, tx.chain]); - expect(dbTxs).toEqual([[tx.txId, null, tx.network]]); - }); - - /** - * @target DatabaseHandler.insertColdStorageTx should NOT insert tx when - * there is already an advanced tx for the chain - * @dependencies - * - database - * @scenario - * - mock two transactions - * - insert one of the txs into db (with `inSign` status) - * - run test (call `insertTx`) - * - check database - * @expected - * - tx should NOT be inserted into db - */ - it('should NOT insert tx when there is already an advanced tx for the chain', async () => { - // mock two transactions - const chain = 'chain'; - const tx1 = TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - const tx2 = TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - - // insert one of the txs into db - await DatabaseHandlerMock.insertTxRecord(tx2, TransactionStatus.inSign); - - // run test - await DatabaseHandler.insertTx(tx1, requiredSign); - - // tx should NOT be inserted into db - const dbTxs = (await DatabaseHandlerMock.allTxRecords()).map( - (tx) => tx.txId - ); - expect(dbTxs).toEqual([tx2.txId]); - }); - - /** - * @target DatabaseHandler.insertColdStorageTx should insert tx when - * txId is lower than existing approved tx - * @dependencies - * - database - * @scenario - * - mock two transactions - * - insert tx with higher txId (with `approved` status) - * - run test (call `insertTx`) - * - check database - * @expected - * - tx should be inserted into db - */ - it('should insert tx when txId is lower than existing approved tx', async () => { - // mock two transactions - const chain = 'chain'; - const txs = [ - TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ), - TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ), - ]; - let highTx: PaymentTransaction; - let lowTx: PaymentTransaction; - if (txs[0].txId < txs[1].txId) { - highTx = txs[1]; - lowTx = txs[0]; - } else { - highTx = txs[0]; - lowTx = txs[1]; - } - - // insert tx with higher txId (with `approved` status) - await DatabaseHandlerMock.insertTxRecord( - highTx, - TransactionStatus.approved - ); - - // run test - await DatabaseHandler.insertTx(lowTx, requiredSign); - - // tx should be inserted into db - const dbTxs = (await DatabaseHandlerMock.allTxRecords()).map( - (tx) => tx.txId - ); - expect(dbTxs).toEqual([lowTx.txId]); - }); - - /** - * @target DatabaseHandler.insertColdStorageTx should NOT insert tx when - * txId is higher than existing approved tx - * @dependencies - * - database - * @scenario - * - mock two transactions - * - insert tx with lower txId (with `approved` status) - * - run test (call `insertTx`) - * - check database - * @expected - * - tx should NOT be inserted into db - */ - it('should NOT insert tx when txId is higher than existing approved tx', async () => { - // mock two transactions - const chain = 'chain'; - const txs = [ - TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ), - TxTestData.mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ), - ]; - let highTx: PaymentTransaction; - let lowTx: PaymentTransaction; - if (txs[0].txId < txs[1].txId) { - highTx = txs[1]; - lowTx = txs[0]; - } else { - highTx = txs[0]; - lowTx = txs[1]; - } - - // insert tx with lower txId (with `approved` status) - await DatabaseHandlerMock.insertTxRecord( - lowTx, - TransactionStatus.approved - ); - - // run test - await DatabaseHandler.insertTx(highTx, requiredSign); - - // tx should NOT be inserted into db - const dbTxs = (await DatabaseHandlerMock.allTxRecords()).map( - (tx) => tx.txId - ); - expect(dbTxs).toEqual([lowTx.txId]); - }); - /** * @target DatabaseHandler.insertColdStorageTx should update failedInSign when * tx is already in database diff --git a/tests/handlers/ChainHandler.mock.ts b/tests/handlers/ChainHandler.mock.ts index b0f3586c..3855454a 100644 --- a/tests/handlers/ChainHandler.mock.ts +++ b/tests/handlers/ChainHandler.mock.ts @@ -133,6 +133,20 @@ class ChainHandlerMock { return this.getMockedChain(chainName)[name]; }; + /** + * mocks a function for mocked chain and return the mock object + * @param chainName mocked chain name + * @param name function name + */ + static mockAndGetChainFunction = ( + chainName: string, + name: string + ): Mock => { + const chain = this.getMockedChain(chainName); + chain[name] = vi.fn(); + return chain[name]; + }; + /** * returns a mocked function object * @param name function name diff --git a/tests/verification/RequestVerifier.spec.ts b/tests/verification/RequestVerifier.spec.ts index d06a0543..adf133d5 100644 --- a/tests/verification/RequestVerifier.spec.ts +++ b/tests/verification/RequestVerifier.spec.ts @@ -482,56 +482,6 @@ describe('RequestVerifier', () => { expect(result).toEqual(true); }); - /** - * @target RequestVerifier.verifyColdStorageTransactionRequest should return false - * when there is already an active cold storage tx for chain in db - * @dependencies - * - TransactionVerifier - * - database - * @scenario - * - mock transaction - * - insert another cold storage transaction into db (same chain) - * - mock TransactionVerifier.verifyColdStorageTransaction - * - run test - * - verify returned value - * @expected - * - returned value should be false - */ - it('should return false when there is already an active cold storage tx for chain in db', async () => { - // mock event and transaction - const chain = 'chain'; - const paymentTx = mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - - // insert another cold storage transaction into db (different chain) - const anotherTx = mockPaymentTransaction( - TransactionType.coldStorage, - chain, - '' - ); - await DatabaseActionMock.insertTxRecord( - anotherTx, - TransactionStatus.approved - ); - - // mock TransactionVerifier.verifyColdStorageTransaction - vi.spyOn( - TransactionVerifier, - 'verifyColdStorageTransaction' - ).mockResolvedValue(true); - - // run test - const result = await RequestVerifier.verifyColdStorageTransactionRequest( - paymentTx - ); - - // verify returned value - expect(result).toEqual(false); - }); - /** * @target RequestVerifier.verifyColdStorageTransactionRequest should return false * when transaction doesn't satisfy cold storage tx conditions diff --git a/tests/verification/TransactionVerifier.spec.ts b/tests/verification/TransactionVerifier.spec.ts index 5b6662b8..9773f3ab 100644 --- a/tests/verification/TransactionVerifier.spec.ts +++ b/tests/verification/TransactionVerifier.spec.ts @@ -1462,5 +1462,310 @@ describe('TransactionVerifier', () => { // verify returned value expect(result).toEqual(false); }); + + /** + * @target TransactionVerifier.verifyColdStorageTransaction should return true + * when active cold storage txs are transferring other tokens + * @dependencies + * - ChainHandler + * - database + * @scenario + * - mock transaction + * - insert a cold storage transaction into database + * - mock ChainHandler + * - mock `extractTransactionOrder` implementation + * - mock `getLockAddressAssets` + * - mock `getChainConfigs` + * - run test + * - verify returned value + * @expected + * - returned value should be true + */ + it('should return true when active cold storage txs are transferring other tokens', async () => { + const chain = CARDANO_CHAIN; + // mock transaction + const tx = mockPaymentTransaction(TransactionType.coldStorage, chain); + // insert a cold storage transaction into database + const coldTx = mockPaymentTransaction(TransactionType.coldStorage, chain); + await DatabaseActionMock.insertTxRecord( + coldTx, + TransactionStatus.approved + ); + // mock ChainHandler `getChain` + ChainHandlerMock.mockChainName(chain); + // mock `extractTransactionOrder` implementation + const coldAddress = `coldAddress`; + const order: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000000n, + tokens: [ + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 100000000000n, + }, + ], + }, + }, + ]; + const activeColdTxOrder: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000n, + tokens: [ + { + id: 'd2f6eb37450a3d568de93d623e69bd0ba1238daacc883d75736abd23.527374457267565465737432', + value: 100000000000n, + }, + ], + }, + }, + ]; + const mockedExtractTransactionOrder = + ChainHandlerMock.mockAndGetChainFunction( + chain, + 'extractTransactionOrder' + ); + mockedExtractTransactionOrder.mockImplementation((givenTx: any) => { + if (givenTx.txId === tx.txId) return order; + else if (givenTx.txId === coldTx.txId) return activeColdTxOrder; + else + throw Error( + `'extractTransactionOrder' is not mocked for txId [${givenTx.txId}]` + ); + }); + // mock `getLockAddressAssets` + const lockedAssets: AssetBalance = { + nativeToken: 1100000000n, + tokens: [ + { + id: 'd2f6eb37450a3d568de93d623e69bd0ba1238daacc883d75736abd23.527374457267565465737432', + value: 225000000000n, + }, + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 500000000000n, + }, + ], + }; + ChainHandlerMock.mockChainFunction( + chain, + 'getLockAddressAssets', + lockedAssets, + true + ); + // mock `getChainConfigs` + ChainHandlerMock.mockChainFunction(chain, 'getChainConfigs', { + addresses: { cold: coldAddress }, + }); + + // run test + const result = await TransactionVerifier.verifyColdStorageTransaction(tx); + + // verify returned value + expect(result).toEqual(true); + }); + + /** + * @target TransactionVerifier.verifyColdStorageTransaction should return false + * when there is an active cold storage tx that is transferring the same token + * @dependencies + * - ChainHandler + * - database + * @scenario + * - mock transaction + * - insert a cold storage transaction into database + * - mock ChainHandler + * - mock `extractTransactionOrder` implementation + * - mock `getLockAddressAssets` + * - mock `getChainConfigs` + * - run test + * - verify returned value + * @expected + * - returned value should be false + */ + it('should return false when there is an active cold storage tx that is transferring the same token', async () => { + const chain = CARDANO_CHAIN; + // mock transaction + const tx = mockPaymentTransaction(TransactionType.coldStorage, chain); + // insert a cold storage transaction into database + const coldTx = mockPaymentTransaction(TransactionType.coldStorage, chain); + await DatabaseActionMock.insertTxRecord( + coldTx, + TransactionStatus.approved + ); + // mock ChainHandler `getChain` + ChainHandlerMock.mockChainName(chain); + // mock `extractTransactionOrder` implementation + const coldAddress = `coldAddress`; + const order: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000000n, + tokens: [ + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 100000000000n, + }, + ], + }, + }, + ]; + const activeColdTxOrder: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000n, + tokens: [ + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 100000000000n, + }, + ], + }, + }, + ]; + const mockedExtractTransactionOrder = + ChainHandlerMock.mockAndGetChainFunction( + chain, + 'extractTransactionOrder' + ); + mockedExtractTransactionOrder.mockImplementation((givenTx: any) => { + if (givenTx.txId === tx.txId) return order; + else if (givenTx.txId === coldTx.txId) return activeColdTxOrder; + else + throw Error( + `'extractTransactionOrder' is not mocked for txId [${givenTx.txId}]` + ); + }); + // mock `getLockAddressAssets` + const lockedAssets: AssetBalance = { + nativeToken: 1100000000n, + tokens: [ + { + id: 'd2f6eb37450a3d568de93d623e69bd0ba1238daacc883d75736abd23.527374457267565465737432', + value: 225000000000n, + }, + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 500000000000n, + }, + ], + }; + ChainHandlerMock.mockChainFunction( + chain, + 'getLockAddressAssets', + lockedAssets, + true + ); + // mock `getChainConfigs` + ChainHandlerMock.mockChainFunction(chain, 'getChainConfigs', { + addresses: { cold: coldAddress }, + }); + + // run test + const result = await TransactionVerifier.verifyColdStorageTransaction(tx); + + // verify returned value + expect(result).toEqual(false); + }); + + /** + * @target TransactionVerifier.verifyColdStorageTransaction should return false + * when there is an active cold storage tx that is transferring the native token + * @dependencies + * - ChainHandler + * - database + * @scenario + * - mock transaction + * - insert a cold storage transaction into database + * - mock ChainHandler + * - mock `extractTransactionOrder` implementation + * - mock `getLockAddressAssets` + * - mock `getChainConfigs` + * - run test + * - verify returned value + * @expected + * - returned value should be false + */ + it('should return false when there is an active cold storage tx that is transferring the native token', async () => { + const chain = CARDANO_CHAIN; + // mock transaction + const tx = mockPaymentTransaction(TransactionType.coldStorage, chain); + // insert a cold storage transaction into database + const coldTx = mockPaymentTransaction(TransactionType.coldStorage, chain); + await DatabaseActionMock.insertTxRecord( + coldTx, + TransactionStatus.approved + ); + // mock ChainHandler `getChain` + ChainHandlerMock.mockChainName(chain); + // mock `extractTransactionOrder` implementation + const coldAddress = `coldAddress`; + const order: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000000n, + tokens: [], + }, + }, + ]; + const activeColdTxOrder: PaymentOrder = [ + { + address: coldAddress, + assets: { + nativeToken: 1000000000n, + tokens: [], + }, + }, + ]; + const mockedExtractTransactionOrder = + ChainHandlerMock.mockAndGetChainFunction( + chain, + 'extractTransactionOrder' + ); + mockedExtractTransactionOrder.mockImplementation((givenTx: any) => { + if (givenTx.txId === tx.txId) return order; + else if (givenTx.txId === coldTx.txId) return activeColdTxOrder; + else + throw Error( + `'extractTransactionOrder' is not mocked for txId [${givenTx.txId}]` + ); + }); + // mock `getLockAddressAssets` + const lockedAssets: AssetBalance = { + nativeToken: 1100000000n, + tokens: [ + { + id: 'd2f6eb37450a3d568de93d623e69bd0ba1238daacc883d75736abd23.527374457267565465737432', + value: 225000000000n, + }, + { + id: 'bb2250e4c589539fd141fbbd2c322d380f1ce2aaef812cd87110d61b.527374434f4d4554565465737432', + value: 500000000000n, + }, + ], + }; + ChainHandlerMock.mockChainFunction( + chain, + 'getLockAddressAssets', + lockedAssets, + true + ); + // mock `getChainConfigs` + ChainHandlerMock.mockChainFunction(chain, 'getChainConfigs', { + addresses: { cold: coldAddress }, + }); + + // run test + const result = await TransactionVerifier.verifyColdStorageTransaction(tx); + + // verify returned value + expect(result).toEqual(false); + }); }); });