Fix Informational Issues

[b-yap]

closes for #257

This PR addresses the ff. items:

• SYT-03 | Unnecessary Variable

  • from audit:

    • The linked variable account_id is only used once in the function. Thus, its value can be supplied directly instead of stored in a variable.

  • files covered:

    • system.rs

• SRL-01 | Usage of Hard-coded Strings

  • from audit:

    • It is a good practice to declare and use Rust constants for such cases to better track their usage and avoid mistakes and bugs that may arise during several different development iterations.

  • files covered:

    • primitives/src/lib.rs
    • clients/
      • vault/tests/helper/constants.rs
      • wallet/src/
        • stellar_wallet.rs
        • horizon/responses.rs

• PAL-03 | Usage of Magic Numbers

  • from audit:

    • We advise the team to declare constants to improve code maintainability and readability. E.g. the client could declare: HOURS_DURING_DAY , MINUTES_IN_HOUR , SECONDS_IN_HOUR, SECONDS_DURING_DAY, etc.

  • files covered:

    • pallets/
      • issue/src/lib.rs
      • redeem/src/lib.rs

• PAL-02 | Unnecessary Result<...> Return Type

  • from audit:

    • methods return Result<..., DispatchError> but that Error will never be thrown.

  • files covered:

    • pallets/
      • replace/src/
        • types.rs
        • lib.rs
      • redeem/src/
        • types.rs
        • lib.rs

• CLI-02 | Typos

  • from audit:

    • "insertin" should be "inserting".
    • "Oracle returned error" should be "Oracle returned an error".
    • "Not fetching missing envelopes from archive for slot {:?}, because on testnet" should be "Not fetching missing
    • envelopes from archive for slot {:?}, because it is on testnet"
    • "the slot where the txset is to get" should be "the slot from where we get the txset"

  • files covered:

    • clients/
      • runtime/src/shutdown.rs
      • vault/src/oracle/collector
        • collector.rs
        • proof_builder.rs

• 9B2-03 | Unused Errors

  • from audit:

    • The errors HttpPostError and SeqNoParsingError are defined in vault/src/error.rs but are not used inside such crate.
    • The same thing happens for the errors NoOrganizationsRegisteredForNetwork, NoValidatorsRegisteredForNetwork, InvalidTransactionSet and InvalidTransactionXDR from stellar-relay/src/lib.rs.

  • files covered:

    • clients/vault/src/error.rs
    • pallets/stellar-relay-src/lib.rs

• 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring (partial)

  • from audit:

    • The function execute_open_requests() has over 150 LOC and does different things at once. This approach is not recommended as it increases the complexity of the whole function, may introduce errors and decreases the code's readability.
    • The last portion of the function is in charge of executing all the remaining requests on the Hashmap. As this functionality can be done in isolation, it is advised that it is moved to a separate function that receives the hashmap or list of open transactions pending execution.

  • files covered:

    • the whole clients/vault/src/requests directory, which replaces execution.rs
    • took the initiative to breakdown vault_integration_tests.rs as well:
      • see clients/vault/tests/helper directory

• STL-03 | Lack of Validation for destination_address on send_payment_to_address() (partial)

  • from audit:

    • The function send_payment_to_address() from stellar_wallet doesn't check if the destination address is the same user. Although this would not lead to malicious behavior, the action doesn't make sense and would make the user lose money in the fees involved in the transfer.

  • files covered:

    • clients/wallet/src/stellar_wallet.rs
      • see my comment on why I exclude the checking for feature testing-utils.

additional PRs:

• 257 FIx Informational Issues - Logic Should be Moved To an Separate Function - Refactoring #388
• 257 Fix informational issues part 3 #390
• 257 fix informational issues part 4 #393
  • 401 fix issues of connecting to stellar overlay network on testnet #414

[b-yap]

mod requests replaces mod execution. It splits the old execution.rs into 3:

• execution.rs
• helper.rs
• structs.rs

[b-yap]

the simple derive debug prints the value of the fields in Vec<u8>. Converting them to a readable string will make it easier when we want to view the xdr.

[b-yap]

see fn listen_for_new_transactions(...) in clients/wallet/src/horizon/horizon.rs

[ebma]

Now that I look and compare this call to spawn_tasks_to_execute_open_requests_async to spawn_tasks_to_pay_and_execute_open_requests, it's striking that one of these functions is passing the vault_id_manager and the other is just using the wallet.
This reveals a potential logical flaw because, although at the moment all the vaults that simultaneously live in the same vault client use the same Stellar wallet, this could change in the future and then we miss fetching the wallet per vault in this function. I think we shouldn't even pass a wallet parameter to the execute_open_requests function in the first place. It should be enough to pass the vault_id_manager like we do already and then iterating over all registered vaults with vault_id_manager.get_entries(), accessing their stored StellarWallet (in the spawn_tasks_to_execute_open_requests_async function).

[b-yap]

good idea. I would prefer a new issue ticket for this.

[ebma]

Good point. I'll create a follow-up

[ebma]

Created the follow-up here.

[ebma]

LGTM. We really need to create a follow-up ticket to clean up the code that is commented out though

[ebma]

We tested all the commands of the workflow locally and it works fine. I don't want to wait for the CI jobs to finish again so we merge this right away.