Conversation
rpanic
left a comment
rpanic
left a comment
There was a problem hiding this comment.
The comment above, and also let's get the CI working, then i'd say its gtg
# Conflicts: # packages/persistance/src/services/prisma/PrismaTransactionStorage.ts
Remove tx hook fail for messages
kadirchan
left a comment
kadirchan
left a comment
There was a problem hiding this comment.
reviewed with my limited knowledge about codebase but looks good overall. Added couple things
| } | ||
|
|
||
| public async removeTxs(included: string[], dropped: string[]) { | ||
| await this.transactionStorage.removeTx(included, "included"); |
There was a problem hiding this comment.
if (type === "dropped") check in removeTx() method makes this call redundant, we can refactor removeTxs() if there is a need of dropping for included ones or remove that call.
Mentioned removeTx()
There was a problem hiding this comment.
While what you're saying is true - its only the case for the PrismaTransactionStorage, not for the InMemoryTransactionStorage. This is because they are implemented differently, and for the inmemory version, we will need to remove them explicitely (while for prisma, the DB schema takes care of it via foreign key relationships)
| limit | ||
| ) | ||
| : txs; | ||
| : txs.slice(0, limit); |
There was a problem hiding this comment.
I think it would be better if we add limit parameter to getPendingUserTransactions method with db level limiting instead and slicing after getting all transactions. This could be a performance problem as txs grows
|
|
||
| public async removeTx(hashes: string[]) { | ||
| this.queue = this.queue.filter((tx) => { | ||
| const hash = tx.hash().toString(); | ||
| return !hashes.includes(hash); | ||
| }); | ||
| } |
There was a problem hiding this comment.
we can reduce lookup complexity in here
| public async removeTx(hashes: string[]) { | |
| this.queue = this.queue.filter((tx) => { | |
| const hash = tx.hash().toString(); | |
| return !hashes.includes(hash); | |
| }); | |
| } | |
| public async removeTx(hashes: string[]) { | |
| const hashSet = new Set(hashes) | |
| this.queue = this.queue.filter((tx) => { | |
| const hash = tx.hash().toString(); | |
| return !hashSet.has(hash); | |
| }); | |
| } |
There was a problem hiding this comment.
also would be better caching that set
| } | ||
|
|
||
| // Under these conditions we want the tx removed from the mempool. | ||
| public async removeTransactionWhen({ |
There was a problem hiding this comment.
It might be a silly question, but in the current version I couldn't be sure whether the transaction gets removed from the mempool when there isn't enough gas fee. Also, an option could be added for after a certain timeout period.
There was a problem hiding this comment.
Added it, thanks for spotting this!
3 participants
Closes #321
Currently, when a tx hook fails the tx is not removed from the mempool. In some cases this is what's needed. However, in others, like when a nonce is too small, this is a problem as the tx will persist in the memppool indefinitely, occupying resources. The solution is to incorporate a new check into the txHook abstract class, called
removeTransactionWhen, which when satisfied causes the tx to be removed from the mempool.