Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp(erc20): align metadata query errors with ERC20 contracts #2073

Merged
merged 4 commits into from Nov 24, 2023

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Nov 23, 2023

…ies implemented

Description

This PR aligns the response from failing metadata queries with the ERC20 contracts which don't expose their metadata. These return the general "execution reverted" error with no further info, which is what we've implemented here.

@MalteHerrmann MalteHerrmann requested a review from a team as a code owner November 23, 2023 18:18
@MalteHerrmann MalteHerrmann requested review from facs95 and 0xstepit and removed request for a team November 23, 2023 18:18
@MalteHerrmann MalteHerrmann changed the title imp(erc20): align metadata query errors with ERC20 contracts without metadata quer… imp(erc20): align metadata query errors with ERC20 contracts Nov 23, 2023
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #2073 (d530ca3) into main (2130f61) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head d530ca3 differs from pull request most recent head 6ff92b8. Consider uploading reports for the commit 6ff92b8 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2073      +/-   ##
==========================================
+ Coverage   70.40%   70.41%   +0.01%     
==========================================
  Files         344      344              
  Lines       25642    25651       +9     
==========================================
+ Hits        18052    18063      +11     
+ Misses       6656     6654       -2     
  Partials      934      934              
Files Coverage Δ
precompiles/erc20/errors.go 36.66% <100.00%> (+20.66%) ⬆️
precompiles/erc20/query.go 96.00% <100.00%> (+0.13%) ⬆️

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM, I suggest using errors.Is(err) instead of using strings.Contains

@MalteHerrmann MalteHerrmann enabled auto-merge (squash) November 24, 2023 07:57
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

LGTM!

@MalteHerrmann MalteHerrmann merged commit 3c22225 into main Nov 24, 2023
26 of 28 checks passed
@MalteHerrmann MalteHerrmann deleted the malte/adjust-metadata-errors branch November 24, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants