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

fix: handle estimated fee error #11671

Merged

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Mar 20, 2024

The error coming from geth and returned the value of the error with a notification.

I have a couple of questions as I'm not sure what the expected outcome is, I'd be happy to discuss it and get some help.

  • Should I handle every incoming error differently and provide a differently interpreted message or returning the initial error value is sufficient?
  • I'm not sure how to test in this case, I couldn't seem to find any examples on how/where the function is used.

Closes #3283

@tomasklim tomasklim self-assigned this Apr 3, 2024
@tomasklim tomasklim requested a review from AdamSchinzel April 3, 2024 14:30
@tomasklim
Copy link
Member

related to #3283

Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

And also please rebase, there were bigger changes to this file.

@tomasklim tomasklim assigned enjojoy and unassigned tomasklim Apr 4, 2024
@AdamSchinzel AdamSchinzel marked this pull request as ready for review April 5, 2024 09:04
@enjojoy enjojoy requested a review from MiroslavProchazka as a code owner May 7, 2024 23:42
@enjojoy
Copy link
Contributor Author

enjojoy commented May 7, 2024

And also please rebase, there were bigger changes to this file.

I believe it's up to date with the develop branch, I tried to rebase it and got no conflicts whatsoever, could you please check?

@enjojoy enjojoy requested a review from AdamSchinzel May 7, 2024 23:45
@MiroslavProchazka
Copy link
Contributor

And also please rebase, there were bigger changes to this file.

I believe it's up to date with the develop branch, I tried to rebase it and got no conflicts whatsoever, could you please check?

It seems that there are some conflicts in packages/suite/src/actions/wallet/send/sendFormEthereumActions.ts

Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

Just some nitpics

suite-common/toast-notifications/src/types.ts Outdated Show resolved Hide resolved
@AdamSchinzel
Copy link
Contributor

Also please reword your commits so they are like fix(suite):..., feat(suite): ... etc.

@enjojoy enjojoy force-pushed the fix/catch-error-fro-blockbook-geth branch 2 times, most recently from 8145595 to 9e263d3 Compare May 10, 2024 15:31
@enjojoy enjojoy requested a review from AdamSchinzel May 10, 2024 15:32
@enjojoy enjojoy force-pushed the fix/catch-error-fro-blockbook-geth branch from 9e263d3 to e5f2464 Compare May 10, 2024 15:35
Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

Nice good job!

@tomasklim
Copy link
Member

tomasklim commented May 10, 2024

Screenshot 2024-05-10 at 21 45 17
Screenshot 2024-05-10 at 21 54 18

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

@Hermez-cz User gets Estimated fee error + reason of error. However, we set gas limit for him to some big number hardcoded in codebase.

I think user can be quite confused that he got an error but he is still able to continue. I guess we should improve the message to inform that we also set some default for him.

@enjojoy enjojoy requested a review from AdamSchinzel May 12, 2024 21:06
@enjojoy enjojoy force-pushed the fix/catch-error-fro-blockbook-geth branch from 4f6d963 to 1eed85e Compare May 13, 2024 22:35
@enjojoy enjojoy force-pushed the fix/catch-error-fro-blockbook-geth branch from 1eed85e to 3187243 Compare May 14, 2024 19:31
Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

LGTM

@AdamSchinzel AdamSchinzel enabled auto-merge (squash) May 14, 2024 20:55
@AdamSchinzel AdamSchinzel merged commit 32a42b1 into trezor:develop May 14, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ETH: catch error from blockbook/geth in estimateFee
5 participants