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: remove app-layer usage of transport error #363

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Mar 22, 2024

Motivation

TransportError should capture Transport-layer errors, not application or RPC semantics errors. RPC semantics errors should be captured by RpcError (note that TransportErrorKind instantiates an RpcError<TransportError>, not a TransportError). Application errors should be captured by either RpcError (e.g. when the application expects a non-null response) or by an ErrResp

Solution

  • Remove usages of TransportErrorKing::custom_str
  • Remove MissingReceipt variant from TransportError
  • Add NullResp variant to RpcError

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich prestwich added bug Something isn't working debt Tech debt which needs to be addressed labels Mar 22, 2024
@prestwich prestwich self-assigned this Mar 22, 2024
@prestwich prestwich force-pushed the prestwich/fix-transport-error branch from 5f686fc to 4926852 Compare March 22, 2024 05:48
.ok_or(RpcError::NullResp)?
.header
.base_fee_per_gas
.ok_or(RpcError::UnsupportedFeature("eip1559"))?;
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense

Err(err)
}
}
Err(RpcError::UnsupportedFeature("eip1559")) => self.handle_legacy_tx(tx).await,
Copy link
Member

Choose a reason for hiding this comment

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

I think this could also fail with a
-32601, message: Method not found

rpc error for the fee_history call?

some providers also return -32600, message: Unsupported method:

so perhaps we should be less restrictive here and also check the rpc error response?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could assume any error response is also a reason to retry as a legacy

i'm not 100% convinced that retrying as legacy is the correct thing to do here, as forcing legacy could conflict with users who have already put blobs onto their transaction request. We may want to just return errors here

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, so the unsupported feature check is okay because this looks at the base fee value of the header

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to write down somewhere that 1559 is somewhat privileged by the library in ways that other tx types and HF features are not. I.e. we generally assume that 1559 is active and proactively attempt to use it in gas estimation even tho we acknowledge that there may be chains that don't support it

@prestwich prestwich merged commit 3108462 into main Mar 22, 2024
16 checks passed
@prestwich prestwich deleted the prestwich/fix-transport-error branch March 22, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working debt Tech debt which needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants