Consider token as good when node doesn't support trace calls #3002
Conversation
| @@ -434,6 +435,10 @@ impl OrderValidating for OrderValidator { | |||
| if let TokenQuality::Bad { reason } = self | |||
| .bad_token_detector | |||
| .detect(token) | |||
| .instrument(tracing::info_span!( | |||
There was a problem hiding this comment.
I'm a bit surprised by all these instrumented futures and think these could lead to duplicated tracing spans. Wouldn't it be sufficient to instrument only the inner.detect() calls inside the CachingDetector?
There was a problem hiding this comment.
This would assume we always use a CachingDetector in our estimating chain (which is true now but might change when and we don't want the observability to change in that case). I'm fine with this, but I thought the callsite that creates the outmost
There was a problem hiding this comment.
Actually, I found InstrumentedBadTokenDetector which lends itself perfectly for this.
| @@ -20,17 +21,15 @@ pub async fn trace_many(requests: Vec<CallRequest>, web3: &Web3) -> Result<Vec<B | |||
| serde_json::to_value(vec![TraceType::Trace])?, | |||
| ]) | |||
| }) | |||
| .collect::<Result<Vec<_>>>()?; | |||
| .collect::<Result<Vec<_>>>() | |||
| .map_err(|e| Error::Decoder(e.to_string()))?; | |||
There was a problem hiding this comment.
Looks like we can only hit this .map_err() if our JSON serialization fails so using the Error::Decoder variant seems odd. OTOH web3 doesn't offer a more suitable error variant anyway.
| .context("trace_many")?; | ||
| let traces = match trace_many::trace_many(request, &self.web3).await { | ||
| Ok(result) => result, | ||
| Err(web3::Error::Transport(e)) => { |
There was a problem hiding this comment.
How do we know we want to catch this type of Error? For example, I would rather expect RPCError with ErrorCode::MethodNotFound (-32601)
There was a problem hiding this comment.
If you received Transport Error in practice instead, this might mean the specific node provider has unexpected handling of this error, so this code change would be effective only for that node provider and not for the rest.
In that case I would suggest also adding Rpc variant and handle it similarly as Err(web3::Error::Transport(e))
There was a problem hiding this comment.
Very good point. Seems to be an Alchemy specific behavior (Nodereal returns with the response code you expected). Will add this case.
There was a problem hiding this comment.
LGTM, I am also concern about duplicate spans as @MartinquaXD mentioned.
Description
Not all of our fallback nodes support the
trace_callAPI. While we can configure the tracing URL separately, in case of outage the API may not be able to classify any tokens as good and thus effectively prevent trading.Changes
How to test
both with a Node that does and does not support the trace call API. Observe that in the case of non-support all tokens are of quality good.