-
Notifications
You must be signed in to change notification settings - Fork 138
rfq+rfqmsg: add structured price oracle error codes #1766
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
Open
jtobin
wants to merge
13
commits into
lightninglabs:main
Choose a base branch
from
jtobin:oracle-error-codes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
3a7986f
rfqmsg: add reject-with-custom-message utility
jtobin f58d19e
rfq: add structured oracle error codes
jtobin 2f096e2
rfq: relay error code in oracle response
jtobin b9b018a
rfq: customize reject message by oracle error
jtobin bbd3afe
taprpc: update priceoraclerpc proto defs
jtobin 5274aaf
rfq: refine error code handling
jtobin e875f5a
basic-price-oracle: specify error code
jtobin 6bdb36a
docs: add release notes
jtobin aef763a
rfq: fix linter nit
jtobin 6130dac
rfq: rename oracle error code values
jtobin 79308a2
rfqmsg: use const/enum reject error codes
jtobin 6ff7217
taprpc: update priceoraclerpc proto definitions
jtobin 5eaf8a9
rfq: preserve context in oracle query errors
jtobin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||||||
package rfq | ||||||||||
|
||||||||||
import ( | ||||||||||
"errors" | ||||||||||
"fmt" | ||||||||||
"sync" | ||||||||||
"time" | ||||||||||
|
@@ -27,6 +28,29 @@ const ( | |||||||||
DefaultAcceptPriceDeviationPpm = 50_000 | ||||||||||
) | ||||||||||
|
||||||||||
// QueryError represents an error with additional context about the price | ||||||||||
// oracle query that led to it. | ||||||||||
type QueryError struct { | ||||||||||
// Err is the error returned from a query attempt, possibly from a | ||||||||||
// price oracle. | ||||||||||
Err error | ||||||||||
|
||||||||||
// Context is the context of the price oracle query that led to the | ||||||||||
// error. | ||||||||||
Context string | ||||||||||
} | ||||||||||
|
||||||||||
// Error returns a human-readable version of the QueryError, implementing the | ||||||||||
// main error interface. | ||||||||||
func (err *QueryError) Error() string { | ||||||||||
// If there's no context, just fall back to the wrapped error. | ||||||||||
if err.Context == "" { | ||||||||||
return err.Err.Error() | ||||||||||
} | ||||||||||
// Otherwise prepend the context. | ||||||||||
return err.Context + ": " + err.Err.Error() | ||||||||||
} | ||||||||||
|
||||||||||
// NegotiatorCfg holds the configuration for the negotiator. | ||||||||||
type NegotiatorCfg struct { | ||||||||||
// PriceOracle is the price oracle that the negotiator will use to | ||||||||||
|
@@ -141,22 +165,29 @@ func (n *Negotiator) queryBuyFromPriceOracle(assetSpecifier asset.Specifier, | |||||||||
counterparty, metadata, intent, | ||||||||||
) | ||||||||||
if err != nil { | ||||||||||
return nil, fmt.Errorf("failed to query price oracle for "+ | ||||||||||
"buy price: %w", err) | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: err, | ||||||||||
Context: "failed to query price oracle for buy price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// Now we will check for an error in the response from the price oracle. | ||||||||||
// If present, we will convert it to a string and return it as an error. | ||||||||||
// If present, we will relay it with context. | ||||||||||
if oracleResponse.Err != nil { | ||||||||||
return nil, fmt.Errorf("failed to query price oracle for "+ | ||||||||||
"buy price: %s", oracleResponse.Err) | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: oracleResponse.Err, | ||||||||||
Context: "failed to query price oracle for buy price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// By this point, the price oracle did not return an error or a buy | ||||||||||
// price. We will therefore return an error. | ||||||||||
if oracleResponse.AssetRate.Rate.ToUint64() == 0 { | ||||||||||
return nil, fmt.Errorf("price oracle did not specify a " + | ||||||||||
"buy price") | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: errors.New("price oracle didn't specify " + | ||||||||||
"a price"), | ||||||||||
Context: "failed to query price oracle for buy price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// TODO(ffranr): Check that the buy price is reasonable. | ||||||||||
|
@@ -277,22 +308,29 @@ func (n *Negotiator) querySellFromPriceOracle(assetSpecifier asset.Specifier, | |||||||||
counterparty, metadata, intent, | ||||||||||
) | ||||||||||
if err != nil { | ||||||||||
return nil, fmt.Errorf("failed to query price oracle for "+ | ||||||||||
"sell price: %w", err) | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: err, | ||||||||||
Context: "failed to query price oracle for sell price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// Now we will check for an error in the response from the price oracle. | ||||||||||
// If present, we will convert it to a string and return it as an error. | ||||||||||
// If present, we will relay it with context. | ||||||||||
if oracleResponse.Err != nil { | ||||||||||
return nil, fmt.Errorf("failed to query price oracle for "+ | ||||||||||
"sell price: %s", oracleResponse.Err) | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: oracleResponse.Err, | ||||||||||
Context: "failed to query price oracle for sell price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// By this point, the price oracle did not return an error or a sell | ||||||||||
// price. We will therefore return an error. | ||||||||||
if oracleResponse.AssetRate.Rate.Coefficient.ToUint64() == 0 { | ||||||||||
return nil, fmt.Errorf("price oracle did not specify an " + | ||||||||||
"asset to BTC rate") | ||||||||||
return nil, &QueryError{ | ||||||||||
Err: errors.New("price oracle didn't specify " + | ||||||||||
"a price"), | ||||||||||
Context: "failed to query price oracle for sell price", | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// TODO(ffranr): Check that the sell price is reasonable. | ||||||||||
|
@@ -372,10 +410,12 @@ func (n *Negotiator) HandleIncomingBuyRequest( | |||||||||
peerID, request.PriceOracleMetadata, IntentRecvPayment, | ||||||||||
) | ||||||||||
if err != nil { | ||||||||||
// Send a reject message to the peer. | ||||||||||
// Construct an appropriate RejectErr based on | ||||||||||
// the oracle's response, and send it to the | ||||||||||
// peer. | ||||||||||
msg := rfqmsg.NewReject( | ||||||||||
request.Peer, request.ID, | ||||||||||
rfqmsg.ErrUnknownReject, | ||||||||||
createCustomRejectErr(err), | ||||||||||
) | ||||||||||
sendOutgoingMsg(msg) | ||||||||||
|
||||||||||
|
@@ -473,10 +513,12 @@ func (n *Negotiator) HandleIncomingSellRequest( | |||||||||
peerID, request.PriceOracleMetadata, IntentPayInvoice, | ||||||||||
) | ||||||||||
if err != nil { | ||||||||||
// Send a reject message to the peer. | ||||||||||
// Construct an appropriate RejectErr based on | ||||||||||
// the oracle's response, and send it to the | ||||||||||
// peer. | ||||||||||
msg := rfqmsg.NewReject( | ||||||||||
request.Peer, request.ID, | ||||||||||
rfqmsg.ErrUnknownReject, | ||||||||||
createCustomRejectErr(err), | ||||||||||
) | ||||||||||
sendOutgoingMsg(msg) | ||||||||||
|
||||||||||
|
@@ -495,6 +537,35 @@ func (n *Negotiator) HandleIncomingSellRequest( | |||||||||
return nil | ||||||||||
} | ||||||||||
|
||||||||||
// createCustomRejectErr creates a RejectErr with code 0 and a custom message | ||||||||||
// based on an error response from a price oracle. | ||||||||||
func createCustomRejectErr(err error) rfqmsg.RejectErr { | ||||||||||
var queryError *QueryError | ||||||||||
// Check if the error we've received is the expected QueryError, and | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. newline before the comments
Suggested change
|
||||||||||
// return an opaque rejection error if not. | ||||||||||
if !errors.As(err, &queryError) { | ||||||||||
return rfqmsg.ErrUnknownReject | ||||||||||
} | ||||||||||
|
||||||||||
var oracleError *OracleError | ||||||||||
// Check if the QueryError contains the expected OracleError, and | ||||||||||
// return an opaque rejection error if not. | ||||||||||
if !errors.As(queryError, &oracleError) { | ||||||||||
return rfqmsg.ErrUnknownReject | ||||||||||
} | ||||||||||
|
||||||||||
switch oracleError.Code { | ||||||||||
// The price oracle has indicated that it doesn't support the asset, | ||||||||||
// so return a rejection error indicating that. | ||||||||||
case UnsupportedAssetOracleErrorCode: | ||||||||||
return rfqmsg.ErrRejectWithCustomMsg(oracleError.Msg) | ||||||||||
// The error code is either unspecified or unknown, so return an | ||||||||||
// opaque rejection error. | ||||||||||
default: | ||||||||||
return rfqmsg.ErrUnknownReject | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// HandleOutgoingSellOrder handles an outgoing sell order by constructing sell | ||||||||||
// requests and passing them to the outgoing messages channel. These requests | ||||||||||
// are sent to peers. | ||||||||||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an enum val from our lib that we can use here instead of
1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If you agree, I'll drop the commit that changes the mock oracle before merge.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we want to go ahead and update this price oracle it should include a version bump too.
Having said that, I believe it's okay to just leave the
Code
field unspecified and let it default to0
, in order to not have to populate it with magic numbersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think updating the mock oracle in a separate PR makes sense to me. We can wait until 0.7 for example.