Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

changed failedHash to hashError #817

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Conversation

fguthmann
Copy link
Contributor

@fguthmann fguthmann commented Jul 13, 2023

Modified SyscallHandlerError to have a hash error of type HashError

Description

Modify the error to be clearer and give more information about the type of hash error we get.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
  • Documentation has been added/updated.

@fguthmann fguthmann linked an issue Jul 13, 2023 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Merging #817 (a015b7a) into main (20206ef) will decrease coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #817      +/-   ##
==========================================
- Coverage   91.72%   91.68%   -0.04%     
==========================================
  Files          53       54       +1     
  Lines       12421    12428       +7     
==========================================
+ Hits        11393    11395       +2     
- Misses       1028     1033       +5     
Impacted Files Coverage Δ
src/core/errors/contract_address_errors.rs 0.00% <0.00%> (ø)
src/core/errors/hash_errors.rs 0.00% <0.00%> (ø)
src/syscalls/syscall_handler_errors.rs 100.00% <ø> (ø)
src/utils.rs 94.31% <0.00%> (ø)
src/core/transaction_hash/mod.rs 80.00% <75.00%> (-0.65%) ⬇️
src/hash_utils.rs 100.00% <100.00%> (ø)

Copy link
Contributor

@matias-gonz matias-gonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! One comment: the hash_utils module should return HashErrors not SyscallHandler Errors, the error mapping into SyscallHandlerError should be done after calling the function

@fguthmann fguthmann marked this pull request as ready for review July 14, 2023 12:56
@fguthmann
Copy link
Contributor Author

Added this translation function in ContractAdressError, is this the right way to go from ContractAddressError to HashError

`impl From<HashError> for ContractAddressError {

    fn from(error: HashError) -> Self {
        ContractAddressError::HashError(error)
    }
}`

Copy link
Contributor

@matias-gonz matias-gonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Could you implement a From<HashError> for SyscallHandlerError?
If not that's okay too 👍

Copy link
Collaborator

@SantiagoPittella SantiagoPittella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple more places where we can change the error type and use the HashError. But I think that it's good enough now. We can revisit this later.

Congrats.

@SantiagoPittella SantiagoPittella added this pull request to the merge queue Jul 14, 2023
Merged via the queue into main with commit b1d67ce Jul 14, 2023
@juanbono juanbono deleted the change_error_type_hash_utils branch July 31, 2023 23:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change error type for hash_utils module
4 participants