refactor: centralise delegate call verification in verifier #2427
Conversation
| const getMultisigTransactionUrl = `${chain.transactionService}/api/v1/multisig-transactions/${proposeTransactionDto.safeTxHash}/`; | ||
| const getContractUrlPattern = `${chain.transactionService}/api/v1/contracts/`; | ||
| networkService.get.mockImplementation(({ url }) => { | ||
| if (url === getChainUrl) { | ||
| return Promise.resolve({ data: rawify(chain), status: 200 }); | ||
| } | ||
| if (url === getSafeUrl) { |
There was a problem hiding this comment.
Nit: We could change this if statement and the one after to else if for better readability/performance.
There was a problem hiding this comment.
As other mocks here use early returns, I suggest we sooner unify the implementation across the entire project in a follow up. There are places that use switch, if/else if and early returns. What do you think?
There was a problem hiding this comment.
In my opinion performance is not a problem here, as each if statement makes the mock to return early. Anyway, I agree we should unify these mock routers across the entire project. For the sake of readability and considering they use to have many branches, I'd go with switch statements, but I also think this would be a low value/priority refactor, wdyt?
| safe, | ||
| }); | ||
| if ( | ||
| !transaction.confirmations || |
There was a problem hiding this comment.
We are building with confirmation and the minLength of the confirmation faker is 1 which means there must be at least 1 confirmation. Why do we need to check here whether a confirmation exist or not?
There was a problem hiding this comment.
This is to prevent TypeScript errors. I'm currently working on increased test coverage where this is entriely removed though.
Edit: I've opened the PR removing these across all suites.
| safe, | ||
| }); | ||
| if ( | ||
| !transaction.confirmations || |
There was a problem hiding this comment.
Same as before, I won’t mark any further occurrences of such checks in the code.
|
LGTM, but the PR has some conflicts with the main branch that would possibly require a refactor and the test adjustment 🤔 |
I'll hold off on merging this until #2431 is finished as there are tests marked as todo there that require changes here. |
This has now been addressed in accordance with my previous comment. |
Summary
We refine confirmations and hashes of transaction proposal, confirmation and that from our API. We also block delegate calls by default or based on their trust. This was added as part of verification but it is not within the verifier.
This moves the relevant logic into the verifier, thereby centralising all logic in one place.
Changes