Skip to content
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

Merged
merged 36 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
1c7138e
WIP: incorporate call params into fee
dwasse Oct 15, 2024
76c1d6f
WIP: decompose getFee()
dwasse Oct 15, 2024
8aeb626
Feat: account for CallValue in getDestinationFee()
dwasse Oct 15, 2024
a42cef9
Fix: build
dwasse Oct 15, 2024
43da983
Feat: test call value and call param calculation in fee pricer
dwasse Oct 16, 2024
73531fd
Feat: add context on request body in rpc fwd errs
dwasse Oct 17, 2024
9baed17
Fix: zap estimate gas
dwasse Oct 18, 2024
b123b0f
Cleanup: move gas estimate into own func
dwasse Oct 18, 2024
4e448e8
Fix: quoter tests
dwasse Oct 18, 2024
62d16f7
Cleanup: lint
dwasse Oct 21, 2024
1376b7b
Cleanup: lint
dwasse Oct 21, 2024
2620aaf
Fix: tests
dwasse Oct 21, 2024
32df025
Cleanup: decompose func
dwasse Oct 21, 2024
b9cd947
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
dwasse Oct 21, 2024
cfb8d3c
Cleanup: lint
dwasse Oct 21, 2024
cebce1e
Fix: tests
dwasse Oct 21, 2024
3186071
Cleanup: lint
dwasse Oct 21, 2024
af05543
Feat: always use quote fee multiplier
dwasse Oct 24, 2024
ae348be
WIP: abi encode pack relay()
dwasse Oct 24, 2024
842a0f6
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
dwasse Oct 25, 2024
f8d6d4f
Feat: pass full RawRequest for gas estimation
dwasse Oct 25, 2024
9de9cfb
Cleanup: lint
dwasse Oct 25, 2024
a97da60
Fix: pricer tests
dwasse Oct 25, 2024
308e66b
Feat: ignore static l2 fee when incorporating call params
dwasse Oct 25, 2024
d081602
Fix: tests
dwasse Oct 25, 2024
7e6e1fc
Clarifying comment
dwasse Oct 25, 2024
596d105
Feat: add extra check for call param len
dwasse Oct 25, 2024
7a72ad4
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
dwasse Nov 14, 2024
9fde9a8
Attempt to fix flake
dwasse Nov 14, 2024
a9b3a18
Cleanup: lint
dwasse Nov 14, 2024
2a132a8
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
dwasse Nov 14, 2024
9833e0d
Fix: build
dwasse Nov 14, 2024
7d47ab0
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
dwasse Nov 14, 2024
f93b3b3
feat(rfq-relayer): apply zap fee to dest amount for active quotes [SL…
dwasse Nov 14, 2024
2510a2b
fix(rfq-relayer): gas estimation for zaps (#3413)
dwasse Dec 3, 2024
d9db854
Merge branch 'feat/relayer-arb-call' into feat/arb-call-fee-pricer
parodime Dec 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ethergo/backends/anvil/anvil.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"github.com/synapsecns/sanguine/core/dockerutil"
"github.com/synapsecns/sanguine/core/mapmutex"
"github.com/synapsecns/sanguine/core/processlog"
"github.com/synapsecns/sanguine/core/retry"
"github.com/synapsecns/sanguine/ethergo/backends"
"github.com/synapsecns/sanguine/ethergo/backends/base"
"github.com/synapsecns/sanguine/ethergo/chain"
Expand Down Expand Up @@ -386,9 +387,15 @@
var acct *keystore.Key
// TODO handle storing accounts to conform to get tx context
if address != nil {
acct = f.GetAccount(*address)
if acct == nil {
f.T().Errorf("could not get account %s", address.String())
err := retry.WithBackoff(ctx, func(ctx context.Context) error {

Check failure on line 390 in ethergo/backends/anvil/anvil.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
acct = f.GetAccount(*address)
if acct == nil {
return fmt.Errorf("could not get account %s", address.String())
}
return nil

Check warning on line 395 in ethergo/backends/anvil/anvil.go

View check run for this annotation

Codecov / codecov/patch

ethergo/backends/anvil/anvil.go#L390-L395

Added lines #L390 - L395 were not covered by tests
}, retry.WithMaxTotalTime(10*time.Second))
if err != nil {
f.T().Errorf("could not get account %s: %v", address.String(), err)

Check warning on line 398 in ethergo/backends/anvil/anvil.go

View check run for this annotation

Codecov / codecov/patch

ethergo/backends/anvil/anvil.go#L397-L398

Added lines #L397 - L398 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve retry mechanism implementation

While adding retry logic for account retrieval is a good practice, there are several areas for improvement:

  1. The retry function doesn't use its context parameter
  2. The timeout duration is hardcoded
  3. The error handling could be more informative
  4. The changes lack test coverage

Consider applying these improvements:

-		err := retry.WithBackoff(ctx, func(ctx context.Context) error {
+		err := retry.WithBackoff(ctx, func(retryCtx context.Context) error {
+			select {
+			case <-retryCtx.Done():
+				return retryCtx.Err()
+			default:
				acct = f.GetAccount(*address)
				if acct == nil {
-					return fmt.Errorf("could not get account %s", address.String())
+					return fmt.Errorf("account %s not found in backend store", address.String())
				}
				return nil
+			}
-		}, retry.WithMaxTotalTime(10*time.Second))
+		}, retry.WithMaxTotalTime(10*time.Second), retry.WithDescription("get account"))
 		if err != nil {
-			f.T().Errorf("could not get account %s: %v", address.String(), err)
+			f.T().Errorf("failed to get account %s after retries: %v", address.String(), err)
 			return res
 		}

Also, consider adding the following test:

func TestGetTxContextWithRetry(t *testing.T) {
    ctx := context.Background()
    backend := setupTestBackend(t)
    
    // Test retry mechanism
    address := common.HexToAddress("0x123")
    result := backend.GetTxContext(ctx, &address)
    
    // Verify result
    assert.Equal(t, backends.AuthType{}, result, "Should return empty AuthType for non-existent account")
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests


[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests

🪛 GitHub Check: Lint (ethergo)

[failure] 390-390:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

return res
}
} else {
Expand Down
7 changes: 4 additions & 3 deletions services/omnirpc/proxy/forward.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"context"
"crypto/sha256"
"fmt"
goHTTP "net/http"
"strings"

"github.com/ImVexed/fasturl"
"github.com/goccy/go-json"
"github.com/jftuga/ellipsis"
Expand All @@ -14,8 +17,6 @@
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
"golang.org/x/exp/slices"
goHTTP "net/http"
"strings"
)

type rawResponse struct {
Expand Down Expand Up @@ -56,7 +57,7 @@

standardizedResponse, err = standardizeResponse(ctx, &f.rpcRequest[0], rpcMessage)
if err != nil {
return nil, fmt.Errorf("could not standardize response: %w", err)
return nil, fmt.Errorf("could not standardize response from body %s: %w", ellipsis.Shorten(string(body), 200), err)

Check warning on line 60 in services/omnirpc/proxy/forward.go

View check run for this annotation

Codecov / codecov/patch

services/omnirpc/proxy/forward.go#L60

Added line #L60 was not covered by tests
}
}

Expand Down
4 changes: 3 additions & 1 deletion services/rfq/e2e/rfq_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func (i *IntegrationSuite) TestETHtoETH() {
}

//nolint:gosec
func (i *IntegrationSuite) TestArbitraryCall() {
func (i *IntegrationSuite) TestBridgeZap() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Evaluate the necessity of the //nolint:gosec directive

The //nolint:gosec comment suppresses all gosec linter warnings for the TestBridgeZap function. Unless there is a specific reason to disable these security checks, consider removing this directive to ensure that potential security issues are caught during linting.

// start the relayer and guard
go func() {
_ = i.relayer.Start(i.GetTestContext())
Expand All @@ -403,6 +403,8 @@ func (i *IntegrationSuite) TestArbitraryCall() {
_ = i.guard.Start(i.GetTestContext())
}()

fmt.Printf("omnirpc url: %s\n", i.destBackend.RPCAddress())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unnecessary debug print statement

The line fmt.Printf("omnirpc url: %s\n", i.destBackend.RPCAddress()) appears to be a debug statement used during development. To keep the test output clean and avoid cluttering the logs, consider removing this line.

Apply this diff to remove the debug print:

-	fmt.Printf("omnirpc url: %s\n", i.destBackend.RPCAddress())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fmt.Printf("omnirpc url: %s\n", i.destBackend.RPCAddress())


// load token contracts
const startAmount = 1000
const rfqAmount = 900
Expand Down
185 changes: 159 additions & 26 deletions services/rfq/relayer/pricer/fee_pricer.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,18 @@
"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"
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
)
Expand All @@ -22,9 +28,9 @@
// 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, quoteRequest *reldb.QuoteRequest) (*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, quoteRequest *reldb.QuoteRequest) (*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.
Expand All @@ -44,10 +50,12 @@
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](),
Expand All @@ -63,6 +71,7 @@
clientFetcher: clientFetcher,
handler: handler,
priceFetcher: priceFetcher,
relayerAddress: relayerAddress,
}
}

Expand Down Expand Up @@ -116,7 +125,8 @@
return fee, nil
}

func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination uint32, denomToken string, isQuote bool) (*big.Int, error) {
//nolint:gosec
func (f *feePricer) GetDestinationFee(parentCtx context.Context, _, destination uint32, denomToken string, isQuote bool, quoteRequest *reldb.QuoteRequest) (*big.Int, error) {
var err error
ctx, span := f.handler.Tracer().Start(parentCtx, "getDestinationFee", trace.WithAttributes(
attribute.Int(metrics.Destination, int(destination)),
Expand All @@ -127,14 +137,28 @@
metrics.EndSpanWithErr(span, err)
}()

// Calculate the destination fee
gasEstimate, err := f.config.GetDestGasEstimate(int(destination))
if err != nil {
return nil, fmt.Errorf("could not get dest gas estimate: %w", err)
fee := big.NewInt(0)

// Calculate the static L2 fee if it won't be incorporated by directly estimating the relay() call
// in addZapFees().
if quoteRequest == nil || quoteRequest.Transaction.CallParams == nil || len(quoteRequest.Transaction.CallParams) == 0 {
gasEstimate, err := f.config.GetDestGasEstimate(int(destination))
if err != nil {
return nil, fmt.Errorf("could not get dest gas estimate: %w", err)
}

Check warning on line 148 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L147-L148

Added lines #L147 - L148 were not covered by tests
fee, err = f.getFee(ctx, destination, destination, gasEstimate, denomToken, isQuote)
if err != nil {
return nil, err
}

Check warning on line 152 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L151-L152

Added lines #L151 - L152 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve CallParams validation

The condition quoteRequest.Transaction.CallParams == nil || len(quoteRequest.Transaction.CallParams) == 0 should be extracted into a helper method for better readability and reuse.

+func hasZapCall(quoteRequest *reldb.QuoteRequest) bool {
+    return quoteRequest != nil && 
+           quoteRequest.Transaction.CallParams != nil && 
+           len(quoteRequest.Transaction.CallParams) > 0
+}

-if quoteRequest == nil || quoteRequest.Transaction.CallParams == nil || len(quoteRequest.Transaction.CallParams) == 0 {
+if !hasZapCall(quoteRequest) {

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 147-148: services/rfq/relayer/pricer/fee_pricer.go#L147-L148
Added lines #L147 - L148 were not covered by tests


[warning] 151-152: services/rfq/relayer/pricer/fee_pricer.go#L151-L152
Added lines #L151 - L152 were not covered by tests

}
fee, err := f.getFee(ctx, destination, destination, gasEstimate, denomToken, isQuote)
if err != nil {
return nil, err
span.SetAttributes(attribute.String("raw_fee", fee.String()))

// If specified, calculate and add the call fee, as well as the call value which will be paid by the relayer
if quoteRequest != nil {
fee, err = f.addZapFees(ctx, destination, denomToken, quoteRequest, fee)
if err != nil {
return nil, err
}

Check warning on line 161 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L160-L161

Added lines #L160 - L161 were not covered by tests
}

// If specified, calculate and add the L1 fee
Expand All @@ -147,11 +171,92 @@
fee = new(big.Int).Add(fee, l1Fee)
span.SetAttributes(attribute.String("l1_fee", l1Fee.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) {
// addZapFees incorporates the cost of the call and the call value into the fee.
// Note that to be conservative, we always use the QuoteFixedFeeMultiplier over the RelayFixedFeeMultiplier.
//
//nolint:gosec
func (f *feePricer) addZapFees(ctx context.Context, destination uint32, denomToken string, quoteRequest *reldb.QuoteRequest, fee *big.Int) (*big.Int, error) {
span := trace.SpanFromContext(ctx)

if len(quoteRequest.Transaction.CallParams) > 0 {
gasEstimate, err := f.getZapGasEstimate(ctx, destination, quoteRequest)
if err != nil {
return nil, err
}

Check warning on line 190 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L189-L190

Added lines #L189 - L190 were not covered by tests
callFee, err := f.getFee(ctx, destination, destination, int(gasEstimate), denomToken, true)
if err != nil {
return nil, err
}

Check warning on line 194 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L193-L194

Added lines #L193 - L194 were not covered by tests
fee = new(big.Int).Add(fee, callFee)
span.SetAttributes(attribute.String("call_fee", callFee.String()))
}

if quoteRequest.Transaction.CallValue != nil && quoteRequest.Transaction.CallValue.Sign() > 0 {
callValueFloat := new(big.Float).SetInt(quoteRequest.Transaction.CallValue)
valueDenom, err := f.getDenomFee(ctx, destination, destination, denomToken, callValueFloat)
if err != nil {
return nil, err
}

Check warning on line 204 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L203-L204

Added lines #L203 - L204 were not covered by tests
valueScaled, err := f.getFeeWithMultiplier(ctx, destination, true, valueDenom)
if err != nil {
return nil, err
}

Check warning on line 208 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L207-L208

Added lines #L207 - L208 were not covered by tests
fee = new(big.Int).Add(fee, valueScaled)
span.SetAttributes(attribute.String("value_scaled", valueScaled.String()))
}

return fee, nil
}

// cache so that we don't have to parse the ABI every time.
var fastBridgeV2ABI *abi.ABI

const methodName = "relay0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be updated to "relayV2" to update to #3410


ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (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)
}

Check warning on line 225 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L224-L225

Added lines #L224 - L225 were not covered by tests

if fastBridgeV2ABI == nil {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
if err != nil {
return 0, fmt.Errorf("could not parse ABI: %w", err)
}

Check warning on line 231 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L230-L231

Added lines #L230 - L231 were not covered by tests
fastBridgeV2ABI = &parsedABI
}
Comment on lines +217 to +234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add thread safety to ABI cache

The global fastBridgeV2ABI variable is accessed without synchronization, which could lead to race conditions in concurrent scenarios.

+var (
+    fastBridgeV2ABI     *abi.ABI
+    fastBridgeV2ABIOnce sync.Once
+)

 func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) {
     // ...
-    if fastBridgeV2ABI == nil {
-        parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
-        if err != nil {
-            return 0, fmt.Errorf("could not parse ABI: %w", err)
-        }
-        fastBridgeV2ABI = &parsedABI
-    }
+    var parseErr error
+    fastBridgeV2ABIOnce.Do(func() {
+        parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
+        if err != nil {
+            parseErr = fmt.Errorf("could not parse ABI: %w", err)
+            return
+        }
+        fastBridgeV2ABI = &parsedABI
+    })
+    if parseErr != nil {
+        return 0, parseErr
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// cache so that we don't have to parse the ABI every time.
var fastBridgeV2ABI *abi.ABI
const methodName = "relay0"
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (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 fastBridgeV2ABI == nil {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
if err != nil {
return 0, fmt.Errorf("could not parse ABI: %w", err)
}
fastBridgeV2ABI = &parsedABI
}
// cache so that we don't have to parse the ABI every time.
var (
fastBridgeV2ABI *abi.ABI
fastBridgeV2ABIOnce sync.Once
)
const methodName = "relay0"
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (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
fastBridgeV2ABIOnce.Do(func() {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
if err != nil {
parseErr = fmt.Errorf("could not parse ABI: %w", err)
return
}
fastBridgeV2ABI = &parsedABI
})
if parseErr != nil {
return 0, parseErr
}


encodedData, err := fastBridgeV2ABI.Pack(methodName, quoteRequest.RawRequest, f.relayerAddress)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing an issue here. I don't think quoteRequest.RawRequest is getting set at all when the active quote is generated:

quoteInput := QuoteInput{
OriginChainID: rfqRequest.Data.OriginChainID,
DestChainID: rfqRequest.Data.DestChainID,
OriginTokenAddr: common.HexToAddress(rfqRequest.Data.OriginTokenAddr),
DestTokenAddr: common.HexToAddress(rfqRequest.Data.DestTokenAddr),
OriginBalance: inv[rfqRequest.Data.OriginChainID][common.HexToAddress(rfqRequest.Data.OriginTokenAddr)],
DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)],
OriginAmountExact: originAmountExact,
}
if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
if !ok {
return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
}
quoteInput.QuoteRequest = &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
ZapNative: zapNative,
ZapData: []byte(rfqRequest.Data.ZapData),
},
}
}

Therefore we don't correctly estimate the gas here. Constructing the full request is somewhat tricky here (e.g. we need to set the destination amount in advance), happy to discuss the details on a call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we actually do supply the RawRequest when doing the EstimateGas() call here:

https://github.com/synapsecns/sanguine/blob/feat/arb-call-fee-pricer/services/rfq/relayer/pricer/fee_pricer.go#L235

The QuoteInput.QuoteRequest is really only used to communicate the extra data around the quoter functions. I considered introducing a new struct for this, maybe simpler to just include these two fields individually within the QuoteInput struct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We supply it, but we never actually set the value for it, so it's always an empty bytes array. Let's figure out the best way to generate it, as the current contracts don't expose the encoding function.

if err != nil {
return 0, fmt.Errorf("could not encode function call: %w", err)
}

Check warning on line 238 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L237-L238

Added lines #L237 - L238 were not covered by tests

rfqAddr, err := f.config.GetRFQAddress(int(destination))
if err != nil {
return 0, fmt.Errorf("could not get RFQ address: %w", err)
}

Check warning on line 243 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L242-L243

Added lines #L242 - L243 were not covered by tests

callMsg := ethereum.CallMsg{
From: f.relayerAddress,
To: &rfqAddr,
Value: quoteRequest.Transaction.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)
}

Check warning on line 254 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L253-L254

Added lines #L253 - L254 were not covered by tests

return gasEstimate, nil
}

func (f *feePricer) GetTotalFee(parentCtx context.Context, origin, destination uint32, denomToken string, isQuote bool, quoteRequest *reldb.QuoteRequest) (_ *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)),
Expand All @@ -170,7 +275,7 @@
))
return nil, err
}
destFee, err := f.GetDestinationFee(ctx, origin, destination, denomToken, isQuote)
destFee, err := f.GetDestinationFee(ctx, origin, destination, denomToken, isQuote, quoteRequest)
if err != nil {
span.AddEvent("could not get destination fee", trace.WithAttributes(
attribute.String("error", err.Error()),
Expand Down Expand Up @@ -202,6 +307,30 @@
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
}

Check warning on line 315 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L314-L315

Added lines #L314 - L315 were not covered by tests

feeScaled, err := f.getFeeWithMultiplier(ctx, gasChain, isQuote, feeDenom)
if err != nil {
return nil, err
}

Check warning on line 320 in services/rfq/relayer/pricer/fee_pricer.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/pricer/fee_pricer.go#L319-L320

Added lines #L319 - L320 were not covered by tests

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use integer arithmetic to prevent precision loss.

The fee calculations use big.Float which can lead to precision loss. Consider using big.Int for all calculations to maintain precision.

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

[warning] 306-307: services/rfq/relayer/pricer/fee_pricer.go#L306-L307
Added lines #L306 - L307 were not covered by tests


[warning] 311-312: services/rfq/relayer/pricer/fee_pricer.go#L311-L312
Added lines #L311 - L312 were not covered by tests

}

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
Expand All @@ -222,7 +351,6 @@

// 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
Expand All @@ -239,6 +367,17 @@
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 {
Expand All @@ -252,22 +391,16 @@
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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)


Comment on lines +407 to +408
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Specify Rounding Behavior When Applying Fee Multiplier

In the getFeeWithMultiplier method, the conversion from big.Float to big.Int truncates towards zero:

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 Ceil method to always round up:

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.
Expand Down
Loading
Loading