feat: add auto gas and fee estimations #3510
Conversation
vgonkivs
requested review from
rach-id and
evan-forbes
and removed request for
a team
WalkthroughThe changes introduce several updates to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant TxClient
participant Signer
participant CosmosSDK
User->>TxClient: SubmitPayForBlob
TxClient->>Signer: Build Transaction with msgs
Signer->>CosmosSDK: Estimate Gas
CosmosSDK-->>Signer: Gas Estimate
Signer-->>TxClient: TxBuilder with Gas
TxClient-->>User: Transaction Result
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
pkg/user/tx_client_test.go (1)
46-68: Consider adding more detailed assertions inTestSubmitPayForBlobto verify the transaction details.It would be beneficial to include assertions that check specific transaction details beyond just the response code and gas wanted, to ensure that the transactions are formed and processed correctly.
pkg/user/tx_client.go (1)
34-34: Document theOptiontype more thoroughly.It would be helpful to add more detailed documentation for the
Optiontype, explaining its purpose and usage within theTxClientconfiguration.
| func (s *Signer) txBuilder(msgs []sdktypes.Msg, opts ...TxOption) (client.TxBuilder, error) { | ||
| builder := s.enc.NewTxBuilder() | ||
| if err := builder.SetMsgs(msgs...); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
| builder = opt(builder) | ||
| } | ||
| return builder | ||
| return builder, nil |
There was a problem hiding this comment.
Refactor txBuilder to simplify error handling and improve readability.
- if err := builder.SetMsgs(msgs...); err != nil {
- return nil, err
- }
+ err := builder.SetMsgs(msgs...)
+ if err != nil {
+ return nil, err
+ }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.
| func (s *Signer) txBuilder(msgs []sdktypes.Msg, opts ...TxOption) (client.TxBuilder, error) { | |
| builder := s.enc.NewTxBuilder() | |
| if err := builder.SetMsgs(msgs...); err != nil { | |
| return nil, err | |
| } | |
| for _, opt := range opts { | |
| builder = opt(builder) | |
| } | |
| return builder | |
| return builder, nil | |
| func (s *Signer) txBuilder(msgs []sdktypes.Msg, opts ...TxOption) (client.TxBuilder, error) { | |
| builder := s.enc.NewTxBuilder() | |
| err := builder.SetMsgs(msgs...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for _, opt := range opts { | |
| builder = opt(builder) | |
| } | |
| return builder, nil |
vgonkivs
force-pushed
the
estimate_gas_and_fee
branch
from
66abdc9
to
9a10b3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
pkg/user/tx_client.go (2)
8-8: Consider documenting the purpose of themathimport.Adding a comment explaining why the
mathpackage is imported could help maintainability, especially for future developers who might wonder about its usage.
Line range hint
381-411: Enhance error handling inConfirmTx.The method for determining whether to retry a transaction based on the error message string is brittle. Consider implementing a more robust mechanism or using error types/codes for this purpose.
Refactor the error handling to use error types or specific error codes instead of relying on string matching, which is prone to breaking with changes in error message text.
| func (client *TxClient) estimateGas(ctx context.Context, txBuilder client.TxBuilder) (uint64, error) { | ||
| _, _, err := client.signer.signTransaction(txBuilder) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| txBytes, err := s.signer.EncodeTx(txBuilder.GetTx()) | ||
| txBytes, err := client.signer.EncodeTx(txBuilder.GetTx()) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| resp, err := sdktx.NewServiceClient(s.grpc).Simulate(ctx, &sdktx.SimulateRequest{ | ||
| resp, err := sdktx.NewServiceClient(client.grpc).Simulate(ctx, &sdktx.SimulateRequest{ | ||
| TxBytes: txBytes, | ||
| }) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| gasLimit := uint64(float64(resp.GasInfo.GasUsed) * s.gasMultiplier) | ||
| gasLimit := uint64(float64(resp.GasInfo.GasUsed) * client.gasMultiplier) |
There was a problem hiding this comment.
Optimize gas estimation logic in estimateGas.
The method could benefit from caching or other optimizations to reduce the overhead of repeated gas estimations, especially in high-throughput scenarios.
Consider implementing caching mechanisms or other performance optimizations to reduce the computational overhead of gas estimation.
pkg/user/tx_client.go
Outdated
| func (client *TxClient) BroadcastTx(ctx context.Context, msgs []sdktypes.Msg, opts ...TxOption) (*sdktypes.TxResponse, error) { | ||
| client.mtx.Lock() | ||
| defer client.mtx.Unlock() | ||
| account, err := client.getAccountNameFromMsgs(msgs) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := s.checkAccountLoaded(ctx, account); err != nil { | ||
| if err := client.checkAccountLoaded(ctx, account); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tx, account, _, err := s.signer.SignTx(msgs, opts...) | ||
| txBuilder, err := client.signer.txBuilder(msgs, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| txBytes, err := s.signer.EncodeTx(tx) | ||
| estimated := false | ||
| for _, coin := range txBuilder.GetTx().GetFee() { | ||
| if coin.Denom == appconsts.BondDenom { | ||
| estimated = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| gasLimit := txBuilder.GetTx().GetGas() | ||
| if float32(gasLimit) < appconsts.DefaultMinGasPrice { | ||
| if !estimated { | ||
| // add at least 1utia as fee to builder as it affects gas calculation. | ||
| txBuilder.SetFeeAmount(sdktypes.NewCoins(sdktypes.NewCoin(appconsts.BondDenom, sdktypes.NewInt(1)))) | ||
| } | ||
| gasLimit, err = client.estimateGas(ctx, txBuilder) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| txBuilder.SetGasLimit(gasLimit) | ||
| } | ||
|
|
||
| if !estimated { | ||
| fee := int64(math.Ceil(appconsts.DefaultMinGasPrice * float64(gasLimit))) | ||
| txBuilder.SetFeeAmount(sdktypes.NewCoins(sdktypes.NewCoin(appconsts.BondDenom, sdktypes.NewInt(fee)))) | ||
| } | ||
|
|
||
| account, _, err = client.signer.signTransaction(txBuilder) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return s.broadcastTx(ctx, txBytes, account) | ||
| txBytes, err := client.signer.EncodeTx(txBuilder.GetTx()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return client.broadcastTx(ctx, txBytes, account) |
There was a problem hiding this comment.
Review complex transaction handling in BroadcastTx.
The function handles multiple aspects of transaction processing, including locking, account loading, gas estimation, and broadcasting. Consider breaking down this function into smaller, more focused functions to enhance readability and maintainability.
Consider refactoring to separate concerns, such as account loading, gas estimation, and transaction broadcasting into distinct methods.
| ) | ||
|
|
||
| const ( | ||
| DefaultPollTime = 3 * time.Second | ||
| DefaultGasMultiplier float64 = 1.1 | ||
| ) | ||
|
|
||
| type Option func(s *TxClient) | ||
| type Option func(client *TxClient) |
There was a problem hiding this comment.
Refactor Option type function signature for clarity.
The change from func(s *TxClient) to func(client *TxClient) improves clarity by using a more descriptive parameter name. This is a good practice as it enhances code readability.
| gasLimit := uint64(float64(types.DefaultEstimateGas(blobSizes)) * client.gasMultiplier) | ||
| fee := uint64(math.Ceil(appconsts.DefaultMinGasPrice * float64(gasLimit))) | ||
| // prepend calculated params, so it can be overwritten in case the user has specified it. | ||
| opts = append([]TxOption{SetGasLimit(gasLimit), SetFee(fee)}, opts...) |
There was a problem hiding this comment.
This currently breaks down if someone wants us to estimate the gas but wants to provide a gas price themselves (if there's congestion for example). We should create an issue and look to address that in the future
There was a problem hiding this comment.
This case works fine, as the gas will be estimated but the calculated gas price will be overwritten. The issue might be if the user wants us to estimate gas price, providing a gas limit. But in this case, I guess we will get an insufficient fee error that is already handled, so the tx will be retried with higher fees.
There was a problem hiding this comment.
Yeah it doesn't quite work as would be intuitive. Ideally, I as a user could set the gas price and request that the client estimates the gas limit and sets the fee by multiplying the gas price I provided with the gas limit that was estimated. This is a bit tricky to do so I think we should follow it up later.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
pkg/user/tx_client.go (1)
Line range hint
381-411: TheConfirmTxmethod's logic to handle transaction confirmation is generally sound. However, the error handling could be more robust to differentiate between different reasons for a transaction not being found.- if !strings.Contains(err.Error(), "not found") { + if !strings.Contains(err.Error(), "not found") // Add more specific error handling here
|
Generally, I'd make a FeeHandler |
Can you elaborate on this? |
|
Can create an issue, describing all ideas so we can have a clear understanding of what should be done and do we need it. |
|
Yes let's create an issue and discuss it further there |
celestia-bot
requested review from
a team and
ramin
and removed request for
a team
Closes #3342
Overview
Some points to consider:
Simulatethat brings additional roundtrips to the estimation process. As we discussed internally with @cmwaters we should try to get rid of roundtrips in favour of static gas estimations(for different types of messages), as it has been done for blob estimation.