Refactor create/processTransaction
#3239
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Deployment links
|
|
E2E Tests Failed Failed tests:
|
katspaugh
force-pushed
the
refactor-tx-fns
branch
from
825d1d4
to
55572f6
Compare
|
The merge conficts can be disregarded in place of the functions here. I discussed the changes I made with @katspaugh and they are already included in the refactor. |
|
|
||
| if (isExecution) { | ||
| dispatch(updateTransactionStatus({ safeTxHash: tx.safeTxHash, status: LocalTransactionStatus.PENDING_FAILED })) | ||
| this.safeTxHash = tx.safeTxHash |
There was a problem hiding this comment.
Why does it have to be a property on the instance of the processor? It seems non-idiomatic from an OOP point of view. I would think about a processor as something that processes transactions and doesn't hold any state-specific to a transaction it processed. Imo such properties should be specific to the transaction instance, and there's one within the function body (txProps). So if it processes a transaction, it would still keep properties from the last processed transaction on the instance. I'm a little struggling to phrase my thoughts, but right now, the instance code is too tied to the previous processed transaction, but it's exported as a general-purpose class that can process/create multiple transactions
There was a problem hiding this comment.
Adding info I discovered when talking with Aaron:
so because the same instance is reused across different transactions I think it's possible that methods that are called later may reference the wrong values. Like this.isExecution in onComplete. If I create a transaction that is supposed to be executed and isExecution is set to true, and after that, I create another one where this.isExecution is false. When onComplete is called for the first transaction this.isExecution will be false
There was a problem hiding this comment.
Haven't tested it though, but since the interface allows such scenario, it could be good to add a test for such cases with subsequent transactions
There was a problem hiding this comment.
Oh, that's very no bueno. Singleton vs instantiating the class for every tx. 🤦
Great catch, thanks Mikhail!
There was a problem hiding this comment.
@usame-algan and I have just adjusted it to instantiate the TxSender with every call. It would be great if you can take another look at it please @mikheevm @katspaugh
src/logic/exceptions/registry.ts
Outdated
| @@ -47,6 +48,7 @@ enum ErrorCodes { | |||
| _811 = '811: Error calculating ERC721 transfer data for adding a Safe owner', | |||
| _812 = '812: Error calculating ERC721 transfer data for removing a Safe owner', | |||
| _813 = '813: Error calculating ERC721 transfer data for replacing a Safe owner', | |||
| _814 = '814: Error performing an offline tx signature', | |||
There was a problem hiding this comment.
nit: it's not offline but off-chain
| @@ -36,6 +37,24 @@ export const getContractErrorMessage = async ({ | |||
| const contractOutput = abi.rawDecode(['string'], returnBuffer.slice(4))[0] | |||
| return decodeMessage(contractOutput) | |||
| } catch (e) { | |||
| return decodeMessage(e.message) | |||
| return null | |||
There was a problem hiding this comment.
just to double-check if we're hiding the error on purpose
There was a problem hiding this comment.
If I understand correctly, this was causing the app to crash. It was being handled incorrectly and couldn't be decoded. @katspaugh, can you confirm?
There was a problem hiding this comment.
Yes, we weren't handling this in any special way because it's not critical and there's no good recovery strategy. Other than logging, which is a good move.
| if (isExecution) { | ||
| dispatch(updateTransactionStatus({ safeTxHash: tx.safeTxHash, status: LocalTransactionStatus.PENDING_FAILED })) | ||
| } | ||
| return |
There was a problem hiding this comment.
I'll add some logging here. Thanks for noticing it!
|
It should be good to go now. Only a few things:
|
Tests are definitely needed. This is such a behemoth though that I'm not sure how we could approach this though. I'd be appreciative of any insight/help regarding this @gnosis/safe-web
I agree with this to some extent but when you compare what we have with what it was before, I think it is at least an improvement on the duplicity and messiness it was. As they are such integral Safe logic, however, I would more than welcome input on how we could improve them further. I'd love for them to be as perfect as they possible could be. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
@francovenica @liliya-soroka merging this one w/o QA for now, we want to continue refactoring this but need it on dev to avoid conflicts. |
What it solves
Resolves #3187
How this PR fixes it
createTransactionandprocessTransactionwere long, convuleted and had a lot of duplicate code. The code was revisted, tidied and refactored.How to test it
All instances where transactions are created/approved are touched by the reafctoring of these two key functions. All methods need to be tested an confirmed that they work.