Refactor tx filters

[marcindsobczak]

Resolves #3623

Changes:

Changing ITxFilter to be like IIncomingTxFilter (returns AddTxResult? instead of string).

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Testing

Requires testing

• Yes
• No

[LukaszRozmej]

We still have both ITxFilter and IIncomingTxFilter, we should unify and simplify. Then delete the obsolete one. That would probably make TxFilterAdapter obsolete too.

This is good first step but not enough for proper refactor.

[LukaszRozmej]

Great work, but few details and improvements can be still done

[LukaszRozmej]

maybe we should use AcceptTxResult further down the line instead of ToString() here? Not sure.

[LukaszRozmej]

We could rename "result" to "accepted" or "allowed" in many places now so it would be more readable with those if's.

[prashantkumar1982]

This PR broke Chainlink integration.
The refactoring of error message broke our regular expression matching logic, which tells us why a Tx failed, and makes our StateMachine work.

A partial fix that Chainlink needed on our side is here: smartcontractkit/chainlink#7989

@marcindsobczak in future, please keep this in mind, and possibly, could you notify Chainlink before changing error response strings?

[marcindsobczak]

Thank you @prashantkumar1982 for a message. We highly appreciate such a feedback, although we don't have a knowledge how your setup works, so it would be hard to catch which changes can possibly affect it. Possible solution could be to add integration tests which we can run on our CI. Could you please provide us such a tests? We would love to add it to our codebase and then it will be easy to remember to ping you every time when we change something and tests start failing

[dimriou]

Hey @marcindsobczak , thanks for the suggestions. We'll discuss this internally and let you know.

For reference, here (https://github.com/smartcontractkit/chainlink/blob/develop/core/chains/evm/client/errors.go#L170) you can find the regexes we use to match Nethermind's RPC responses. Depending on each response, we make a different decision on how to handle our pending transactions. As @prashantkumar1982 mentioned, if the regexes change this means that our Nodes will not be able to handle some of the responses, thus affecting our pending transactions.