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.

Sign up for GitHub

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

refactor: decouple base fee fetching as part of quote from simulation #550

Merged
merged 8 commits into from
Nov 5, 2024
Merged

refactor: decouple base fee fetching as part of quote from simulation #550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
979a958
refactor: return base fee in /quote regardless of simulation success.
p0mvn Nov 4, 2024
a933e0c
separate base fee from simulation. Pre-compute during ingest
p0mvn Nov 4, 2024
052139b
updates
p0mvn Nov 4, 2024
5004eec
updates
p0mvn Nov 4, 2024
dc26ec0
Update router/repository/memory_router_repository.go
p0mvn Nov 4, 2024
5862547
fix
p0mvn Nov 5, 2024
9b1e5dd
separate interface
p0mvn Nov 5, 2024
e68a836
rename
p0mvn Nov 5, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

## Unreleased

- #548 - Return base fee in /quote regardless of simulation success.
- #547 - Add /quote simulation for "out given in" single routes.
- #526 - Refactor gas estimation APIs
- #524 - Claimbot
Expand Down
8 changes: 6 additions & 2 deletions app/sidecar_query_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/osmosis-labs/sqs/domain/cosmos/auth/types"
ingestrpcdelivry "github.com/osmosis-labs/sqs/ingest/delivery/grpc"
ingestusecase "github.com/osmosis-labs/sqs/ingest/usecase"
"github.com/osmosis-labs/sqs/ingest/usecase/plugins/basefee"
orderbookclaimbot "github.com/osmosis-labs/sqs/ingest/usecase/plugins/orderbook/claimbot"
orderbookfillbot "github.com/osmosis-labs/sqs/ingest/usecase/plugins/orderbook/fillbot"
orderbookrepository "github.com/osmosis-labs/sqs/orderbook/repository"
Expand Down Expand Up @@ -217,11 +218,10 @@ func NewSideCarQueryServer(appCodec codec.Codec, config domain.Config, logger lo
}

grpcClient := passthroughGRPCClient.GetChainGRPCClient()
gasCalculator := tx.NewGasCalculator(grpcClient, tx.CalculateGas)
gasCalculator := tx.NewMsgSimulator(grpcClient, tx.CalculateGas, routerRepository)
quoteSimulator := quotesimulator.NewQuoteSimulator(
gasCalculator,
app.GetEncodingConfig(),
txfeestypes.NewQueryClient(grpcClient),
types.NewQueryClient(grpcClient),
config.ChainID,
)
Expand Down Expand Up @@ -324,6 +324,10 @@ func NewSideCarQueryServer(appCodec codec.Codec, config domain.Config, logger lo
}
}

// Unconditionally register the base fee fetcher.
baseFeeFetcherPlugin := basefee.NewEndBlockUpdatePlugin(routerRepository, txfeestypes.NewQueryClient(grpcClient), logger)
ingestUseCase.RegisterEndBlockProcessPlugin(baseFeeFetcherPlugin)

// Register chain info use case as a listener to the pool liquidity compute worker (healthcheck).
poolLiquidityComputeWorker.RegisterListener(chainInfoUseCase)

Expand Down
9 changes: 9 additions & 0 deletions domain/base_fee.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package domain

import "github.com/osmosis-labs/osmosis/osmomath"

// BaseFee holds the denom and current base fee
type BaseFee struct {
Copy link
Collaborator

@deividaspetraitis deividaspetraitis Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we really want to have new abstraction? I think sdk.Coin already would decribe it quite well:

fee := sdk.Coin{Denom: "osmo", Amount: 999}

We can have type alias, but I still personally find it redundant:

Fee = skd.Coin

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. This has a Dec
  2. A separate abstraction is introduced for flexibility of extension (i.e if we need to add another piece of information, we would not need to break 10 different APIs to propagate this through the call stack. Instead, modify a single struct). That was a primary reason that this separate struct was introduced

Denom string
CurrentFee osmomath.Dec
}
58 changes: 35 additions & 23 deletions domain/cosmos/tx/msg_simulator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package tx

import (
"context"
"errors"

cosmosclient "github.com/cosmos/cosmos-sdk/client"
txclient "github.com/cosmos/cosmos-sdk/client/tx"
Expand All @@ -11,8 +12,10 @@ import (
signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/osmosis-labs/osmosis/v26/app/params"
txfeestypes "github.com/osmosis-labs/osmosis/v26/x/txfees/types"
"github.com/osmosis-labs/sqs/domain"
"github.com/osmosis-labs/sqs/domain/keyring"
routerrepo "github.com/osmosis-labs/sqs/router/repository"
"google.golang.org/grpc"

gogogrpc "github.com/cosmos/gogoproto/grpc"
)
Expand All @@ -22,7 +25,6 @@ type MsgSimulator interface {
BuildTx(
ctx context.Context,
keyring keyring.Keyring,
txfeesClient txfeestypes.QueryClient,
encodingConfig params.EncodingConfig,
account *authtypes.BaseAccount,
chainID string,
Expand All @@ -43,19 +45,19 @@ type MsgSimulator interface {
// which is the fee amount in the base denomination.
PriceMsgs(
ctx context.Context,
txfeesClient txfeestypes.QueryClient,
encodingConfig cosmosclient.TxConfig,
account *authtypes.BaseAccount,
chainID string,
msg ...sdk.Msg,
) (uint64, sdk.Coin, error)
) domain.QuotePriceInfo
}

// NewGasCalculator creates a new GasCalculator instance.
func NewGasCalculator(clientCtx gogogrpc.ClientConn, calculateGas CalculateGasFn) MsgSimulator {
// NewMsgSimulator creates a new GasCalculator instance.
func NewMsgSimulator(clientCtx gogogrpc.ClientConn, calculateGas CalculateGasFn, memoryRouterRepository routerrepo.RouterRepository) MsgSimulator {
return &txGasCalulator{
clientCtx: clientCtx,
calculateGas: calculateGas,
clientCtx: clientCtx,
calculateGas: calculateGas,
memoryRouterRepository: memoryRouterRepository,
Copy link
Collaborator

@deividaspetraitis deividaspetraitis Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NOTE: See first this comment.

Can we instead of pulling in routerrepo.RouterRepository define more focused interface, for example:

type FeeSource interface {
	GetBaseFee() domain.BaseFee
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}
}

Expand All @@ -64,16 +66,16 @@ type CalculateGasFn func(clientCtx gogogrpc.ClientConn, txf txclient.Factory, ms

// txGasCalulator is a GasCalculator implementation that uses simulated transactions to calculate gas.
type txGasCalulator struct {
clientCtx gogogrpc.ClientConn
calculateGas CalculateGasFn
clientCtx grpc.ClientConnInterface
calculateGas CalculateGasFn
memoryRouterRepository routerrepo.RouterRepository
}

// BuildTx constructs a transaction using the provided parameters and messages.
// Returns a TxBuilder and any error encountered.
func (c *txGasCalulator) BuildTx(
ctx context.Context,
keyring keyring.Keyring,
txfeesClient txfeestypes.QueryClient,
encodingConfig params.EncodingConfig,
account *authtypes.BaseAccount,
chainID string,
Expand All @@ -90,13 +92,13 @@ func (c *txGasCalulator) BuildTx(
return nil, err
}

gasAdjusted, feecoin, err := c.PriceMsgs(ctx, txfeesClient, encodingConfig.TxConfig, account, chainID, msg...)
if err != nil {
return nil, err
priceInfo := c.PriceMsgs(ctx, encodingConfig.TxConfig, account, chainID, msg...)
if priceInfo.Err != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While PriceInfo abstraction might have some benefits, I am not sure why we changing error type to string, it seems like this abstraction encapsulates some client logic that requires this specific format? I think txGasCalulator should be client indepdendent abstraction.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was changed to string so that we can return this to the client as a JSON response.

Duplicating err error when a string error message is already present feels unnecessary.

Happy for this to be iterated in a follow-up if you're feeling strongly

return nil, errors.New(priceInfo.Err)
}

txBuilder.SetGasLimit(gasAdjusted)
txBuilder.SetFeeAmount(sdk.Coins{feecoin})
txBuilder.SetGasLimit(priceInfo.AdjustedGasUsed)
txBuilder.SetFeeAmount(sdk.Coins{priceInfo.FeeCoin})

sigV2 := BuildSignatures(privKey.PubKey(), nil, account.Sequence)
err = txBuilder.SetSignatures(sigV2)
Expand Down Expand Up @@ -145,23 +147,33 @@ func (c *txGasCalulator) SimulateMsgs(encodingConfig cosmosclient.TxConfig, acco
}

// PriceMsgs implements MsgSimulator.
func (c *txGasCalulator) PriceMsgs(ctx context.Context, txfeesClient txfeestypes.QueryClient, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) (uint64, sdk.Coin, error) {
func (c *txGasCalulator) PriceMsgs(ctx context.Context, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) domain.QuotePriceInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

txGasCalculator is used in other contexts such as claimbot too, should we rename domain.QuotePriceInfo to domain.TxFeeInfo, for example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

baseFee := c.memoryRouterRepository.GetBaseFee()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep backwards compatibility with previous implementation where we are able to compute fee by making request to chain instead of using pre-computed fee value? For example, is there any possible scenario where we might want to recompute fee for each simulation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The base fee can only be updated after processing transactions. As a result, refetching it once per-block is appropriate

if baseFee.CurrentFee.IsNil() || baseFee.CurrentFee.IsZero() {
return domain.QuotePriceInfo{Err: "base fee is zero or nil"}
}
if baseFee.Denom == "" {
return domain.QuotePriceInfo{Err: "base fee denom is empty"}
}

_, gasAdjusted, err := c.SimulateMsgs(
encodingConfig,
account,
chainID,
msg,
)
if err != nil {
return 0, sdk.Coin{}, err
return domain.QuotePriceInfo{Err: err.Error(), BaseFee: baseFee.CurrentFee}
}

feeCoin, err := CalculateFeeCoin(ctx, txfeesClient, gasAdjusted)
if err != nil {
return 0, sdk.Coin{}, err
}
feeAmount := CalculateFeeAmount(baseFee.CurrentFee, gasAdjusted)

return gasAdjusted, feeCoin, nil
return domain.QuotePriceInfo{
AdjustedGasUsed: gasAdjusted,
FeeCoin: sdk.Coin{Denom: baseFee.Denom, Amount: feeAmount},
BaseFee: baseFee.CurrentFee,
Err: "",
}
}

// CalculateGas calculates the gas required for a transaction using the provided transaction factory and messages.
Expand Down
Loading
Loading