`FF23021: EVM reverted` errors during invoke with `idempotencyKey` leaves ops in `Initializing` status about firefly HOT 10 CLOSED

ok - thanks all.

I'm happy to stick with proposal 1:

• We spend the idempotencyKey
• Operation goes to Failed status

Given great input from @awrichar and @Chengxuan above, I propose returning retryable: false as an explicit JSON payload in the body of the error that flows back.

Note that the default will be true if retryable is omitted from the JSON, or is null.

I will begin by implementing this in FFTM, and the ethereum plugin in Core.

I observe that the HTTP transport is actually one-level detached from the code contract between core and connectors, as the plugin code in Go is responsible for the mapping. So the above expresses a convention, rather than a hard requirement.

I did read the suggestion on a HTTP Header, but I believe currently got a contract being in the body of the response payload (for propagating error codes) and I think it makes sense to extend that with another parameter.

from firefly.

Proposal 1

I believe it is a more obvious and explainable behavior, if these situations were to instead immediately transition an operation to Failed status - re-instating the behavior before #1406.

However, that the connector must be responsible for reporting that the nature of the failure to accept the transaction is a Reject of the transaction, rather than a temporary condition.

That will be a complex per-connector decision.
In this specific case in EVMConnect:

• The eth_estimateGas operation returned an error from the blockchain node JSON/RPC interface
• The "Revert reason" was successfully extracted from the response, so we are confident the issue is not transient (if it were transient, it would be retryable and we would want the app to be able to re-submit with the idempotencyKey)

sequence diagram source

from firefly.

I like the general idea of the "Proposed" solution. One question would be whether it should return 500 or 400 after it updates the status to Failed, indicating that it is not a retryable error.

from firefly.

I like the idea of the spelling of "Reject" being returning HTTP Status code 400 👍

from firefly.

Actually, I wonder if there is a tweak here @nguyer - to your great point. That we should propagate the 400 all the way back to the application, and instead of leaving the transaction in Initializing status, we should remove the record before returning the 400. I'll do a tweaked sequence diagram for this, for us to consider.

from firefly.

Counter proposal (2)

NOTE: Eliminated in favor of proposal 1 - see below

sequence diagram source

from firefly.

Greate diagrams @peterbroadhurst 🎉

I like the spelling of "Reject" being returning HTTP Status code 400, may want to make the HTTP status code a list.

Below is my reasoning:

Regarding the logic to decide whether an error from a connector is retry-able, I see the following options:

• introduce a new mechanism that requires code changes in connectors to explicitly opt-in for all situations (delegate the decision-making to the connectors)
• introduce an implicit contract/assumption/convention in Firefly to do the decision-making itself and optionally connectors can update their code to embrace this convention

For the first option, one solution is introducing an HTTP response header for connectors to include when the error is non-retryable.

For the second option:

the spelling of "Reject" being returning HTTP Status code 400

This is an example of implicit contract/assumption/convention

For this specific error case ((EVM reverted due to "bad" input param), treat 400 (one of the HTTP client error codes) as non-retryable error does solve it nicely.

I'm not so sure about other error cases. As we are putting a constraint on which HTTP error code (400) can be returned for initialization failure, this forces connectors to use 400 for all the non-retryable errors that might be better represented using other HTTP error codes. (e.g. 409, 413). Also, an error code like 413 can be a tricky one to override as it's often handled by HTTP middleware.

But it has a big advantage of less code change in the connectors and it can co-exist with solutions for the first option in the future (if we ever need one). 👍

from firefly.

I would lean toward the first proposal, where we move the operation to "failed" state if the gas estimation fails. As you said, this was the previous behavior, and I think it makes sense.

The second proposal, where we actually delete the operation and all evidence of it, seems to be a pretty significant change to FireFly's behavior. I think it would be odd for the operation to exist for a few seconds and then not exist later.

On the choice of error codes... it seems like it should be HTTP 400 or 500. I think 400 makes sense if we can reasonably deduce that the request was invalid for some reason, which I do think is the general case being considered here.

from firefly.

The default-true semantics felt error-prone, so I have actually gone for submissionRejected: true as the API spelling in the FFTM layer.

from firefly.

All of the changes, including the new E2E test in core, and releases of FFTM and EVMConnect, have been individually reviewed and are in mainline.

from firefly.