-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(rfq-relayer): fee pricer considers v2 CallValue and CallParams [SLT-320] #3299
Changes from 8 commits
1c7138e
76c1d6f
8aeb626
a42cef9
43da983
73531fd
9baed17
b123b0f
4e448e8
62d16f7
1376b7b
2620aaf
32df025
b9cd947
cfb8d3c
cebce1e
3186071
af05543
ae348be
842a0f6
f8d6d4f
9de9cfb
a97da60
308e66b
d081602
7e6e1fc
596d105
7a72ad4
9fde9a8
a9b3a18
2a132a8
9833e0d
7d47ab0
f93b3b3
2510a2b
d9db854
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,11 +5,16 @@ import ( | |||||||||
"context" | ||||||||||
"fmt" | ||||||||||
"math/big" | ||||||||||
"strings" | ||||||||||
"time" | ||||||||||
|
||||||||||
"github.com/ethereum/go-ethereum" | ||||||||||
"github.com/ethereum/go-ethereum/accounts/abi" | ||||||||||
"github.com/ethereum/go-ethereum/common" | ||||||||||
"github.com/jellydator/ttlcache/v3" | ||||||||||
"github.com/synapsecns/sanguine/core/metrics" | ||||||||||
"github.com/synapsecns/sanguine/ethergo/submitter" | ||||||||||
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgev2" | ||||||||||
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig" | ||||||||||
"go.opentelemetry.io/otel/attribute" | ||||||||||
"go.opentelemetry.io/otel/trace" | ||||||||||
|
@@ -22,9 +27,9 @@ type FeePricer interface { | |||||||||
// GetOriginFee returns the total fee for a given chainID and gas limit, denominated in a given token. | ||||||||||
GetOriginFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool) (*big.Int, error) | ||||||||||
// GetDestinationFee returns the total fee for a given chainID and gas limit, denominated in a given token. | ||||||||||
GetDestinationFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool) (*big.Int, error) | ||||||||||
GetDestinationFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (*big.Int, error) | ||||||||||
// GetTotalFee returns the total fee for a given origin and destination chainID, denominated in a given token. | ||||||||||
GetTotalFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool) (*big.Int, error) | ||||||||||
GetTotalFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (*big.Int, error) | ||||||||||
// GetGasPrice returns the gas price for a given chainID in native units. | ||||||||||
GetGasPrice(ctx context.Context, chainID uint32) (*big.Int, error) | ||||||||||
// GetTokenPrice returns the price of a token in USD. | ||||||||||
|
@@ -44,10 +49,12 @@ type feePricer struct { | |||||||||
handler metrics.Handler | ||||||||||
// priceFetcher is used to fetch prices from coingecko. | ||||||||||
priceFetcher CoingeckoPriceFetcher | ||||||||||
// relayerAddress is the address of the relayer. | ||||||||||
relayerAddress common.Address | ||||||||||
} | ||||||||||
|
||||||||||
// NewFeePricer creates a new fee pricer. | ||||||||||
func NewFeePricer(config relconfig.Config, clientFetcher submitter.ClientFetcher, priceFetcher CoingeckoPriceFetcher, handler metrics.Handler) FeePricer { | ||||||||||
func NewFeePricer(config relconfig.Config, clientFetcher submitter.ClientFetcher, priceFetcher CoingeckoPriceFetcher, handler metrics.Handler, relayerAddress common.Address) FeePricer { | ||||||||||
gasPriceCache := ttlcache.New[uint32, *big.Int]( | ||||||||||
ttlcache.WithTTL[uint32, *big.Int](time.Second*time.Duration(config.GetFeePricer().GasPriceCacheTTLSeconds)), | ||||||||||
ttlcache.WithDisableTouchOnHit[uint32, *big.Int](), | ||||||||||
|
@@ -63,6 +70,7 @@ func NewFeePricer(config relconfig.Config, clientFetcher submitter.ClientFetcher | |||||||||
clientFetcher: clientFetcher, | ||||||||||
handler: handler, | ||||||||||
priceFetcher: priceFetcher, | ||||||||||
relayerAddress: relayerAddress, | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -116,7 +124,7 @@ func (f *feePricer) GetOriginFee(parentCtx context.Context, origin, destination | |||||||||
return fee, nil | ||||||||||
} | ||||||||||
|
||||||||||
func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination uint32, denomToken string, isQuote bool) (*big.Int, error) { | ||||||||||
func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (*big.Int, error) { | ||||||||||
var err error | ||||||||||
ctx, span := f.handler.Tracer().Start(parentCtx, "getDestinationFee", trace.WithAttributes( | ||||||||||
attribute.Int(metrics.Destination, int(destination)), | ||||||||||
|
@@ -136,6 +144,7 @@ func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination | |||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
span.SetAttributes(attribute.String("raw_fee", fee.String())) | ||||||||||
|
||||||||||
// If specified, calculate and add the L1 fee | ||||||||||
l1ChainID, l1GasEstimate, useL1Fee := f.config.GetL1FeeParams(destination, false) | ||||||||||
|
@@ -147,11 +156,79 @@ func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination | |||||||||
fee = new(big.Int).Add(fee, l1Fee) | ||||||||||
span.SetAttributes(attribute.String("l1_fee", l1Fee.String())) | ||||||||||
} | ||||||||||
|
||||||||||
// If specified, calculate and add the call fee, as well as the call value which will be paid by the relayer | ||||||||||
if tx != nil { | ||||||||||
if tx.CallParams != nil { | ||||||||||
gasEstimate, err := f.getZapGasEstimate(ctx, destination, tx) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
callFee, err := f.getFee(ctx, destination, destination, int(gasEstimate), denomToken, isQuote) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
fee = new(big.Int).Add(fee, callFee) | ||||||||||
span.SetAttributes(attribute.String("call_fee", callFee.String())) | ||||||||||
} | ||||||||||
|
||||||||||
if tx.CallValue != nil { | ||||||||||
callValueFloat := new(big.Float).SetInt(tx.CallValue) | ||||||||||
valueDenom, err := f.getDenomFee(ctx, destination, destination, denomToken, callValueFloat) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
valueScaled, err := f.getFeeWithMultiplier(ctx, destination, isQuote, valueDenom) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
fee = new(big.Int).Add(fee, valueScaled) | ||||||||||
span.SetAttributes(attribute.String("value_scaled", valueScaled.String())) | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
span.SetAttributes(attribute.String("destination_fee", fee.String())) | ||||||||||
return fee, nil | ||||||||||
} | ||||||||||
|
||||||||||
func (f *feePricer) GetTotalFee(parentCtx context.Context, origin, destination uint32, denomToken string, isQuote bool) (_ *big.Int, err error) { | ||||||||||
// cache so that we don't have to parse the ABI every time | ||||||||||
var recipientABI *abi.ABI | ||||||||||
|
||||||||||
const methodName = "fastBridgeTransferReceived" | ||||||||||
|
||||||||||
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (gasEstimate uint64, err error) { | ||||||||||
client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | ||||||||||
if err != nil { | ||||||||||
return 0, fmt.Errorf("could not get client: %w", err) | ||||||||||
} | ||||||||||
|
||||||||||
if recipientABI == nil { | ||||||||||
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeRecipientMetaData.ABI)) | ||||||||||
if err != nil { | ||||||||||
return 0, fmt.Errorf("could not parse ABI: %w", err) | ||||||||||
} | ||||||||||
recipientABI = &parsedABI | ||||||||||
} | ||||||||||
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. Prevent potential data race on The global variable Consider initializing import (
"sync"
// other imports...
)
var (
recipientABI *abi.ABI
recipientABIInit sync.Once
)
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (gasEstimate uint64, err error) {
client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination)))
if err != nil {
return 0, fmt.Errorf("could not get client: %w", err)
}
var parseErr error
recipientABIInit.Do(func() {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeRecipientMetaData.ABI))
if err != nil {
parseErr = fmt.Errorf("could not parse ABI: %w", err)
return
}
recipientABI = &parsedABI
})
if parseErr != nil {
return 0, parseErr
}
encodedData, err := recipientABI.Pack(methodName, tx.DestToken, tx.DestAmount, tx.CallParams)
if err != nil {
return 0, fmt.Errorf("could not encode function call: %w", err)
}
// rest of the function...
} This ensures that |
||||||||||
encodedData, err := recipientABI.Pack(methodName, tx.DestToken, tx.DestAmount, tx.CallParams) | ||||||||||
if err != nil { | ||||||||||
return 0, fmt.Errorf("could not encode function call: %w", err) | ||||||||||
} | ||||||||||
|
||||||||||
callMsg := ethereum.CallMsg{ | ||||||||||
From: f.relayerAddress, | ||||||||||
To: &tx.DestRecipient, | ||||||||||
Value: tx.CallValue, | ||||||||||
Data: encodedData, | ||||||||||
} | ||||||||||
gasEstimate, err = client.EstimateGas(ctx, callMsg) | ||||||||||
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
if err != nil { | ||||||||||
return 0, fmt.Errorf("could not estimate gas: %w", err) | ||||||||||
} | ||||||||||
|
||||||||||
return gasEstimate, nil | ||||||||||
} | ||||||||||
|
||||||||||
func (f *feePricer) GetTotalFee(parentCtx context.Context, origin, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (_ *big.Int, err error) { | ||||||||||
ctx, span := f.handler.Tracer().Start(parentCtx, "getTotalFee", trace.WithAttributes( | ||||||||||
attribute.Int(metrics.Origin, int(origin)), | ||||||||||
attribute.Int(metrics.Destination, int(destination)), | ||||||||||
|
@@ -170,7 +247,7 @@ func (f *feePricer) GetTotalFee(parentCtx context.Context, origin, destination u | |||||||||
)) | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
destFee, err := f.GetDestinationFee(ctx, origin, destination, denomToken, isQuote) | ||||||||||
destFee, err := f.GetDestinationFee(ctx, origin, destination, denomToken, isQuote, tx) | ||||||||||
if err != nil { | ||||||||||
span.AddEvent("could not get destination fee", trace.WithAttributes( | ||||||||||
attribute.String("error", err.Error()), | ||||||||||
|
@@ -202,6 +279,30 @@ func (f *feePricer) getFee(parentCtx context.Context, gasChain, denomChain uint3 | |||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
feeWei := new(big.Float).Mul(new(big.Float).SetInt(gasPrice), new(big.Float).SetFloat64(float64(gasEstimate))) | ||||||||||
|
||||||||||
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
feeDenom, err := f.getDenomFee(ctx, gasChain, denomChain, denomToken, feeWei) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
|
||||||||||
feeScaled, err := f.getFeeWithMultiplier(ctx, gasChain, isQuote, feeDenom) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
|
||||||||||
span.SetAttributes( | ||||||||||
attribute.String("gas_price", gasPrice.String()), | ||||||||||
attribute.String("fee_wei", feeWei.String()), | ||||||||||
attribute.String("fee_denom", feeDenom.String()), | ||||||||||
attribute.String("fee_scaled", feeScaled.String()), | ||||||||||
) | ||||||||||
return feeScaled, nil | ||||||||||
Comment on lines
+316
to
+334
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. Use integer arithmetic to prevent precision loss. The fee calculations use Apply these changes: - feeWei := new(big.Float).Mul(new(big.Float).SetInt(gasPrice), new(big.Float).SetFloat64(float64(gasEstimate)))
+ feeWei := new(big.Int).Mul(gasPrice, big.NewInt(int64(gasEstimate)))
- feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil)
+ scaledMultiplier := new(big.Int).SetInt64(int64(multiplier * 1e18))
+ feeScaled = new(big.Int).Div(new(big.Int).Mul(feeDenom, scaledMultiplier), big.NewInt(1e18)) Also applies to: 393-394 🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||
} | ||||||||||
|
||||||||||
func (f *feePricer) getDenomFee(ctx context.Context, gasChain, denomChain uint32, denomToken string, feeWei *big.Float) (*big.Float, error) { | ||||||||||
span := trace.SpanFromContext(ctx) | ||||||||||
|
||||||||||
nativeToken, err := f.config.GetNativeToken(int(gasChain)) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
|
@@ -222,7 +323,6 @@ func (f *feePricer) getFee(parentCtx context.Context, gasChain, denomChain uint3 | |||||||||
|
||||||||||
// Compute the fee. | ||||||||||
var feeDenom *big.Float | ||||||||||
feeWei := new(big.Float).Mul(new(big.Float).SetInt(gasPrice), new(big.Float).SetFloat64(float64(gasEstimate))) | ||||||||||
if denomToken == nativeToken { | ||||||||||
// Denomination token is native token, so no need for unit conversion. | ||||||||||
feeDenom = feeWei | ||||||||||
|
@@ -239,6 +339,17 @@ func (f *feePricer) getFee(parentCtx context.Context, gasChain, denomChain uint3 | |||||||||
attribute.String("fee_usdc", feeUSDC.String()), | ||||||||||
) | ||||||||||
} | ||||||||||
span.SetAttributes( | ||||||||||
attribute.Float64("native_token_price", nativeTokenPrice), | ||||||||||
attribute.Float64("denom_token_price", denomTokenPrice), | ||||||||||
attribute.Int("denom_token_decimals", int(denomTokenDecimals)), | ||||||||||
) | ||||||||||
|
||||||||||
return feeDenom, nil | ||||||||||
} | ||||||||||
|
||||||||||
func (f *feePricer) getFeeWithMultiplier(ctx context.Context, gasChain uint32, isQuote bool, feeDenom *big.Float) (feeScaled *big.Int, err error) { | ||||||||||
span := trace.SpanFromContext(ctx) | ||||||||||
|
||||||||||
var multiplier float64 | ||||||||||
if isQuote { | ||||||||||
|
@@ -252,22 +363,16 @@ func (f *feePricer) getFee(parentCtx context.Context, gasChain, denomChain uint3 | |||||||||
return nil, fmt.Errorf("could not get relay fixed fee multiplier: %w", err) | ||||||||||
} | ||||||||||
} | ||||||||||
span.SetAttributes( | ||||||||||
attribute.Float64("multiplier", multiplier), | ||||||||||
) | ||||||||||
|
||||||||||
// Apply the fixed fee multiplier. | ||||||||||
// Note that this step rounds towards zero- we may need to apply rounding here if | ||||||||||
// we want to be conservative and lean towards overestimating fees. | ||||||||||
feeUSDCDecimalsScaled, _ := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) | ||||||||||
span.SetAttributes( | ||||||||||
attribute.String("gas_price", gasPrice.String()), | ||||||||||
attribute.Float64("native_token_price", nativeTokenPrice), | ||||||||||
attribute.Float64("denom_token_price", denomTokenPrice), | ||||||||||
attribute.Float64("multplier", multiplier), | ||||||||||
attribute.Int("denom_token_decimals", int(denomTokenDecimals)), | ||||||||||
attribute.String("fee_wei", feeWei.String()), | ||||||||||
attribute.String("fee_denom", feeDenom.String()), | ||||||||||
attribute.String("fee_usdc_decimals_scaled", feeUSDCDecimalsScaled.String()), | ||||||||||
) | ||||||||||
return feeUSDCDecimalsScaled, nil | ||||||||||
feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) | ||||||||||
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. Implement conservative rounding for fee calculations. The current implementation rounds towards zero when applying fee multipliers, which could lead to fee underestimation. Consider rounding up to ensure fees are not underestimated. -feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil)
+feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
+feeFloat.SetMode(big.AwayFromZero)
+feeScaled, _ = feeFloat.Int(nil) 📝 Committable suggestion
Suggested change
|
||||||||||
|
||||||||||
Comment on lines
+407
to
+408
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. Specify Rounding Behavior When Applying Fee Multiplier In the feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) This truncation may lead to underestimating the fee, potentially resulting in a smaller fee than intended. Consider specifying the rounding mode to ensure that the fee is rounded up, preventing undercharging: feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
- feeScaled, _ = feeFloat.Int(nil)
+ feeScaled = new(big.Int)
+ feeFloat.SetMode(big.AwayFromZero)
+ feeFloat.Int(feeScaled) Alternatively, use the feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
+ feeFloat = feeFloat.SetMode(big.AwayFromZero)
+ feeScaled, _ = feeFloat.Int(nil) Ensure that the rounding behavior complies with your fee policy to accurately reflect the intended fees. |
||||||||||
return feeScaled, nil | ||||||||||
} | ||||||||||
|
||||||||||
// getGasPrice returns the gas price for a given chainID in native units. | ||||||||||
|
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.
💡 Codebase verification
Potential Breaking Changes Confirmed
The
FeePricer
interface inservices/rfq/relayer/pricer/fee_pricer.go
has not been updated to include the newtx *fastbridgev2.IFastBridgeV2BridgeTransactionV2
parameter in theGetDestinationFee
andGetTotalFee
methods. However, several implementations and callers are already passing this new parameter, which will lead to compilation errors.Action Items:
FeePricer
interface to include thetx
parameter in bothGetDestinationFee
andGetTotalFee
methods.FeePricer
interface are updated accordingly.tx
argument to prevent breaking changes.Category:
🔗 Analysis chain
Potential Breaking Changes Due to Interface Modification
The method signatures for
GetDestinationFee
andGetTotalFee
in theFeePricer
interface have been updated to include a new parametertx *fastbridgev2.IFastBridgeV2BridgeTransactionV2
. This change may cause breaking changes in any implementations of theFeePricer
interface and any code that calls these methods.Please ensure that all implementations of the
FeePricer
interface are updated to accommodate the new parameter. Additionally, verify that all callers of these methods pass the requiredtx
argument.You can run the following script to identify all implementations and usages of the updated methods:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 76698
Script:
Length of output: 323
Script:
Length of output: 1032
Script:
Length of output: 175