[Utility] Refactor and rename some existing functions related to handling transactions

[Olshansk]

Description

Summary generated by Reviewpad on 02 Jun 23 18:33 UTC

This pull request includes various changes across multiple files. These changes involve adding new interfaces and methods, renaming functions, refactoring code, improving error handling, updating documentation, and making modifications to the mempool and transaction validation logic.

Issue

NA

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Avoid using the utilityUnitOfWork in the rpc module because we do not want to have it run utility's business logic
• Added GetIndexedTransaction to the UtilityModule interface to be able to retrieve an indexed transaction without running the underlying business logic
• Renamed HydrateIdxTx to HandleTransaction in the UtilityUnitOfWorkinterface so its more descriptive of what the function does
• Moved over all the business logic related to getting "signer candidates" into its own file for readability purposes
• Renamed anteHandleMessage to basicValidateTransaction
• Split the logic in basicValidateTransaction into multiple smaller functions for readability and so adding new business logic will be clearer

TODO_IN_THIS_COMMIT:

• Add an E2E test that leverages txProtoBytesToRPCIdxTxs in the RPC module
• Add unit tests for GetIndexedTransaction in the utility module
• Add a few unit tests for basicValidateTransaction

Testing

• make develop_test; if any code changes were made
• make test_e2e on k8s LocalNet; if any code changes were made
• e2e-devnet-test passes tests on DevNet; if any code was changed
• Docker Compose LocalNet; if any major functionality was changed or introduced
• k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[Olshansk]

@h5law @dylanlott @adshmh I marked this as ready for review because all the core business logic changes I intend to have in this PR is complete. This touches some code that all of us are working on, so I want to get this in and avoid more complex merge conflicts in the future.

I left a few TODO_IN_THIS_COMMIT items in the GitHub description regarding tests I need to add and will focus on those tomorrow. Just wanted to start getting 👀 at this in parallel and give all of you a heads-up of the change.

[Olshansk]

@h5law @dylanlott @adshmh I haven't added an E2E test that leverages txProtoBytesToRPCIdxTxs and will do that tomorrow. Might also leave it for a follow-up PR depending on how long the review on this one takes.

Otherwise, this is ready to go.

[codecov]

Codecov Report

Patch coverage: 52.30% and project coverage change: +1.11 🎉

Comparison is base (fe4780b) 30.12% compared to head (12104a8) 31.24%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #771      +/-   ##
==========================================
+ Coverage   30.12%   31.24%   +1.11%     
==========================================
  Files         103      106       +3     
  Lines        8836     8958     +122     
==========================================
+ Hits         2662     2799     +137     
+ Misses       5857     5824      -33     
- Partials      317      335      +18     

Impacted Files	Coverage Δ
persistence/indexer/indexer.go	60.82% <ø> (ø)
utility/types/idx_tx.go	0.00% <ø> (ø)
utility/types/message.go	57.02% <ø> (ø)
utility/unit_of_work/tx_message_handler.go	34.51% <ø> (-1.81%)	⬇️
utility/unit_of_work/uow_leader.go	0.00% <0.00%> (ø)
shared/core/types/error.go	6.15% <33.33%> (+6.15%)	⬆️
utility/unit_of_work/tx_message_signers.go	43.75% <43.75%> (ø)
shared/core/types/signature.go	62.50% <50.00%> (ø)
utility/unit_of_work/transaction.go	45.20% <52.27%> (+3.10%)	⬆️
utility/transaction.go	72.22% <75.00%> (+72.22%)	⬆️
... and 2 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[h5law]

LGTM I think the GetIndexedTransaction simplification is spot on, everything else looks in order,

[adshmh]

Wrapping errors in general makes more sense IMO (although I understand it would not be very consistent with current codebase)

[Olshansk]

I'm not a fan of error.go myself (didn't create it), but we're just staying consistent with this approach we work on #556 to standardize it.