rfq+rfqmsg: add structured price oracle error codes #1766
Conversation
ErrRejectWithCustomMsg constructs a RejectErr with error code 0 and the specified error message.
jtobin
added
error handling
RFQ
Formalizes the 'Code' of an 'OracleError' as an 'OracleErrorCode', and adds a couple of values corresponding to structured error cases.
Ensure the error code is part of an OracleError response, and pass the
error intact in query{Bid,Ask}FromPriceOracle, instead of formatting it
as a string.
Uses the OracleError returned by an oracle to customize the rejection message sent to a peer. If the OracleError contains a "transparent" code that's deemed to be suitable for passing on to a peer, then simply relay that error. Otherwise relay an opaque "unknown reject error" message.
Avoids a blunt cast of the wire code, and also refrains from relaying the full Error() message to the peer.
Simply passes an error code of 1 (unsupported subject asset) where appropriate in the mock oracle. The raw code is used instead of oraclerpc.UNSUPPORTED or similar to avoid dependency changes at present. Also adds the mock oracle's log to gitignore.
jtobin
force-pushed
the
oracle-error-codes
branch
from
0a5e9ab to
6bdb36a
Compare
Pull Request Test Coverage Report for Build 17268575857Details
💛 - Coveralls |
| @@ -326,6 +327,7 @@ func (p *RpcPriceOracleServer) QueryAssetRates(_ context.Context, | |||
| Result: &oraclerpc.QueryAssetRatesResponse_Error{ | |||
| Error: &oraclerpc.QueryAssetRatesErrResponse{ | |||
| Message: "unsupported subject asset", | |||
| Code: 1, | |||
There was a problem hiding this comment.
is there an enum val from our lib that we can use here instead of 1?
There was a problem hiding this comment.
I used a raw '1' at present to avoid updating the mock oracle's dependencies (it looks like there have been other RPC changes since it was last updated as well). Perhaps best to avoid changing the mock oracle for now, and just update it to use the latest RPC definitions in another PR?
There was a problem hiding this comment.
(If you agree, I'll drop the commit that changes the mock oracle before merge.)
rfq/negotiator.go
Outdated
| if oracleResponse.Err != nil { | ||
| return nil, fmt.Errorf("failed to query price oracle for "+ | ||
| "buy price: %s", oracleResponse.Err) | ||
| return nil, oracleResponse.Err |
There was a problem hiding this comment.
I think the additional context that was here previously was useful to distinguish between buy/sell price related error. Is there a good reason for removing the additional context here?
There was a problem hiding this comment.
Agree, yeah. Re-added context in a structured form in 5eaf8a9.
rfq/oracle.go
Outdated
| const ( | ||
| // ErrUnspecifiedOracleError represents the case where the oracle has | ||
| // declined to give a more specific reason for the error. | ||
| ErrUnspecifiedOracleError OracleErrorCode = iota |
There was a problem hiding this comment.
I think we usually start error type names with Err.... Maybe we should use names UnspecifiedOracleErrorCode and UnsupportedAssetOracleErrorCode here.
There was a problem hiding this comment.
Used your suggestion, fixed in 6130dac.
rfqmsg/reject.go
Outdated
| // it with a custom error message. | ||
| func ErrRejectWithCustomMsg(msg string) RejectErr { | ||
| return RejectErr{ | ||
| Code: 0, |
There was a problem hiding this comment.
use enum const here instead of 0
There was a problem hiding this comment.
Fixed in 79308a2. There were also some existing cases in that file that used the raw code, so I fixed those too.
| enum ErrorCode { | ||
| // ERROR_UNSPECIFIED indicates an unspecified error. | ||
| ERROR_UNSPECIFIED = 0; | ||
|
|
||
| // UNSUPPORTED indicates the asset is not supported. | ||
| ERROR_UNSUPPORTED = 1; | ||
| } |
There was a problem hiding this comment.
I think these fields should follow the enum filed names in our code so: UNSUPPORTED_ASSET_ORACLE_ERROR_CODE etc
There was a problem hiding this comment.
Agreed, fixed in 6ff7217.
Changes these to avoid the 'Err..' convention presently reserved for type names.
Defines specific consts for the two RejectErr error codes, and uses those in place of the raw uint8 values.
Changes 'ERROR_UNSPECIFIED' and 'ERROR_SUPPORTED' to more closely match the OracleErrorCode values in rfq/oracle.go.
Introduces a structured QueryError that wraps an arbitrary error
(usually from a price oracle) with arbitrary context, and adjusts
query{Buy,Sell}FromPriceOracle to return these errors where appropriate.
Also adjusts createCustomRejectErr to handle QueryErrors, instead of
OracleErrors.
|
@ffranr: review reminder |
Adds some structure to price oracle error codes, along with some custom handling for them. In particular, a code of '1' now corresponds to an 'unsupported asset' error, which, considered a 'public' error, is forwarded in the customizable message field of the reject messages sent to peers. An error with any other code continues, at present, to be treated as an unspecified error.
Resolves #1749, #1326.
(This is a refinement over the closed #1751, which forwarded errors indiscriminately.)