Add support for decoding Swap Orders in Multisends #1348
Conversation
- Swap Orders inside a Multisend should be decoded in the same way as regular ones. - Detection of a Multisend order happens on the first level of transactions (i.e. it does not detect Swap transactions if a Multisend contains another Multisend). - The `mapSwapOrder` function also changed: * It is no longer restricted to a `MultisigTransaction` or `ModuleTransaction` – this was done to support nested transactions which do not have all the properties of a Multisig or Module transactions. * It no longer maps to a `CustomTransactionInfo` – this is now a decision left for the callers. Instead, the promise is rejected if the swap transaction couldn't be decoded successfully.
Pull Request Test Coverage Report for Build 8522264835Details
💛 - Coveralls |
src/routes/transactions/mappers/common/transaction-info.mapper.ts
Outdated
Show resolved
Hide resolved
| // TODO If we can build a sorted hash map of the transactions, we can avoid iterating all of them | ||
| // as we know the pattern of a Swap Order. |
There was a problem hiding this comment.
Sounds like a good solution, but building the hash map would also imply iterating the full array, wouldn't it? The worst-case scenario is the transactions array does not contain a swap order so the iteration reaches the last element.
Btw, related to it: is it possible to have several swap orders in the same multisend transaction? My understanding here is only one can be present, as the code is returning the first occurrence, is this correct?
There was a problem hiding this comment.
Sounds like a good solution, but building the hash map would also imply iterating the full array, wouldn't it? The worst-case scenario is the
transactionsarray does not contain a swap order so the iteration reaches the last element.
Correct. The Hash Map solution would be mostly to optimise accessing the collection while keeping the iterative properties.
Btw, related to it: is it possible to have several swap orders in the same multisend transaction? My understanding here is only one can be present, as the code is returning the first occurrence, is this correct?
It is. However the interface (and the CGW) will only support decoding of a single transaction in that collection. The expected use case is to have a MultiSend with a Token Approval + Swapping (although the MultiSend can contain anything).
There was a problem hiding this comment.
Great, thanks for clarifying!
| private isCoWSwapOrder( | ||
| transaction: MultisigTransaction | ModuleTransaction, | ||
| ): boolean { | ||
| private isSwapOrder(transaction: { data?: `0x${string}` }): boolean { |
There was a problem hiding this comment.
Very nitpicky: I'm under the impression that mapSwapOrder determines whether a transaction is a swap order or not. So this function name is a bit confusing at first glance, I'd say this one checks the data within the transaction, what about renaming it to isSwapOrderData? Do you think it would make the class API clearer?
Not a strong opinion btw, I left this up to you 🙂
There was a problem hiding this comment.
Very nitpicky: I'm under the impression that
mapSwapOrderdetermines whether a transaction is a swap order or not. So this function name is a bit confusing at first glance, I'd say this one checks the data within the transaction, what about renaming it toisSwapOrderData? Do you think it would make the class API clearer?Not a strong opinion btw, I left this up to you 🙂
The goal of mapSwapOrder is to map a transaction to a Swap Order, it's not really to check if the transaction is a Swap Order although it can be used for that (nullability test).
The isSwapOrder is a utility function which works on the "transaction" level as in, it should work with any object that is a transaction-like (that's why the requirement of the argument is to have an optional data argument).
So in summary:
isSwapOrder– returnstrueif the transaction is a Swap Order.mapSwapOrder– returns theSwapOrderif the transaction was successfully mapped to a Swap Order,nullotherwise.
mapSwapOrderfunction also changed:MultisigTransactionorModuleTransaction– this was done to support nested transactions which do not have all the properties of a Multisig or Module transactions.CustomTransactionInfo– this is now a decision left for the callers. Instead, the promise is rejected if the swap transaction couldn't be decoded successfully.