-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add AutoEstimateGas and GasAdjustmentFactor parameters to estimate gas automatically before broadcasting the tx #847
base: main
Are you sure you want to change the base?
Conversation
…ly estimate gas before broadcasting tx
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce two new parameters, Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
go-client/tx_raw_client.go (1)
229-255
: Excellent addition! Just a minor refactoring suggestion.The new
EstimateGas
function is a great addition that provides automatic gas estimation by simulating the transaction. It has the following merits:
- It returns the provided
gasLimit
whenautoEstimateGas
is false, avoiding unnecessary simulation.- It correctly encodes the transaction and simulates it to get the gas info.
- The estimated gas is calculated by multiplying the gas used with the
gasAdjustmentFactor
, allowing fine-tuning.- It ensures the estimated gas does not exceed the provided
gasLimit
by returning the minimum of the two.- Errors are handled appropriately.
Just a minor refactoring suggestion to improve readability:
Consider extracting the gas estimation logic into a separate function to make
EstimateGas
more concise and readable. Here's a suggested refactoring:func (c *RawTxClient) EstimateGas( txBuilder client.TxBuilder, app *app.App, autoEstimateGas bool, gasAdjustmentFactor float64, gasLimit uint64, ) (uint64, error) { if !autoEstimateGas { return gasLimit, nil } estimatedGas, err := c.simulateTx(txBuilder, app, gasAdjustmentFactor) if err != nil { return 0, err } return min(estimatedGas, gasLimit), nil } func (c *RawTxClient) simulateTx( txBuilder client.TxBuilder, app *app.App, gasAdjustmentFactor float64, ) (uint64, error) { txBytes, err := app.TxConfig().TxEncoder()(txBuilder.GetTx()) if err != nil { return 0, fmt.Errorf("estimate gas, encode tx: %w", err) } gasInfo, _, err := app.Simulate(txBytes) if err != nil { return 0, fmt.Errorf("estimate gas, simulate tx: %w", err) } estimatedGas := uint64(float64(gasInfo.GasUsed) * gasAdjustmentFactor) return estimatedGas, nil }This refactoring makes the
EstimateGas
function more readable by moving the simulation logic into a separatesimulateTx
function. The main flow ofEstimateGas
becomes clearer.What do you think? Let me know if you would like me to submit a separate PR with this refactoring.
docs/developer-docs/docs/build-a-keychain/quick-start.md (1)
213-213
: Replace hard tab with spaces.The use of hard tabs for indentation is generally discouraged as it can cause inconsistent formatting across different editors and platforms. Consider replacing the hard tab at the start of this line with spaces to improve consistency and adhere to common style guidelines.
- GasAdjustmentFactor: 1.2, + GasAdjustmentFactor: 1.2,Tools
Markdownlint
213-213: Column: 1
Hard tabs(MD010, no-hard-tabs)
docs/developer-docs/docs/build-a-keychain/sdk/tutorial.md (1)
Line range hint
1-362
: Excellent tutorial! The content is well-structured, easy to follow, and provides a solid foundation for building a keychain service using the Warden Protocol. 🌟A few minor suggestions:
In the "Setting up the project" section, consider adding a brief explanation of what the
keychain-sdk
andtestify
packages are used for. This can help readers understand the purpose of these dependencies.In the "Creating the main application" section, the placeholder for the
handleKeyRequest
function appears before the completemain.go
code. Consider moving it after the complete code snippet to maintain a logical flow.In the "Running tests" section, consider adding a note about the expected output when running the tests. This can help readers confirm that their tests are running correctly.
Overall, the tutorial is comprehensive and provides a great starting point for developers looking to build a keychain service using the Warden Protocol. Well done! 👏
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- cmd/wardenkms/wardenkms.go (2 hunks)
- docs/developer-docs/docs/build-a-keychain/quick-start.md (1 hunks)
- docs/developer-docs/docs/build-a-keychain/sdk/keychain-sdk.md (1 hunks)
- docs/developer-docs/docs/build-a-keychain/sdk/tutorial.md (2 hunks)
- go-client/tx_raw_client.go (6 hunks)
- keychain-sdk/config.go (1 hunks)
- keychain-sdk/example_keychain_test.go (1 hunks)
- keychain-sdk/internal/writer/writer.go (3 hunks)
- keychain-sdk/keychain.go (1 hunks)
Additional context used
Path-based instructions (10)
keychain-sdk/example_keychain_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"keychain-sdk/config.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/writer/writer.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/keychain.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/developer-docs/docs/build-a-keychain/sdk/keychain-sdk.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"go-client/tx_raw_client.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/developer-docs/docs/build-a-keychain/quick-start.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/developer-docs/docs/build-a-keychain/sdk/tutorial.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
docs/developer-docs/docs/build-a-keychain/quick-start.md
213-213: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (16)
keychain-sdk/example_keychain_test.go (1)
34-34
: Verify the impact of disabling auto gas estimation.Setting
AutoEstimateGas
tofalse
disables automatic gas estimation. While this provides more control, it's important to ensure that manual gas estimation is properly handled in other parts of the system to avoid potential issues.Run the following script to search for manual gas estimation logic in the codebase:
Verification successful
Manual gas estimation is properly handled when auto-estimation is disabled
The codebase analysis reveals that the system is well-equipped to handle manual gas estimation when
AutoEstimateGas
is set to false. TheEstimateGas
function ingo-client/tx_raw_client.go
respects this setting and uses the providedgasLimit
when auto-estimation is disabled. Thekeychain-sdk
and related components are properly configured to work with this setting. No immediate issues or inconsistencies were found with disabling auto gas estimation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for manual gas estimation logic. # Test: Search for gas estimation functions. Expect: Occurrences of manual gas estimation logic. rg --type go -i $'gas|estimate'Length of output: 8782
keychain-sdk/config.go (2)
50-55
: LGTM! TheAutoEstimateGas
flag is a useful addition.The
AutoEstimateGas
flag provides flexibility to automatically estimate the gas required for transactions. This can help:
- Optimize transaction costs by setting the
GasLimit
based on estimated needs.- Prevent transactions from failing due to insufficient gas.
- Avoid overpaying for gas by setting an appropriate limit.
Good job!
57-61
: TheGasAdjustmentFactor
is a thoughtful addition. Nice work!The
GasAdjustmentFactor
provides a safety margin to the estimated gas values. This is beneficial because:
- It helps ensure that transactions have sufficient gas even if the estimate is slightly off.
- It allows fine-tuning the gas estimate by applying a configurable safety buffer.
The comment with the example clearly explains how the adjustment factor works, which is helpful for understanding its purpose.
keychain-sdk/internal/writer/writer.go (3)
28-29
: LGTM!The new field
AutoEstimateGas
follows the Golang naming convention, uses the appropriate type, and has the correct visibility.
30-31
: LGTM!The new field
GasAdjustmentFactor
follows the Golang naming convention, uses the appropriate type, and has the correct visibility.
145-145
: LGTM!The
BuildTx
method call is updated correctly to include the newAutoEstimateGas
andGasAdjustmentFactor
parameters.keychain-sdk/keychain.go (1)
126-129
: LGTM! The changes enhance the transaction handling capabilities.The code segment initializes the
Fees
,AutoEstimateGas
, andGasAdjustmentFactor
properties of thetxWriter
instance using values from the configuration. This allows for more flexible and efficient transaction management:
- The
Fees
property ensures that transaction fees are aligned with the configuration settings.- The
AutoEstimateGas
property enables automatic gas estimation for transactions, which can improve efficiency and reduce the likelihood of transaction failures due to insufficient gas.- The
GasAdjustmentFactor
property allows for more nuanced control over gas estimation.The code segment follows the Uber Golang style guide and is well-formatted.
cmd/wardenkms/wardenkms.go (2)
32-38
: LGTM!The new fields added to the
Config
struct follow the Uber Golang Style Guide and provide useful configuration options for gas estimation and transaction parameters. The use ofenv
tags with default values is a good practice.
62-74
: Looks good!The
keychain.NewApp
instantiation has been updated to use the new configuration fields, which is the expected change. The code follows the Uber Golang Style Guide by using a config struct for initialization and maintaining consistency in field names. The transaction fee is correctly set using thesdk
andmath
packages.docs/developer-docs/docs/build-a-keychain/sdk/keychain-sdk.md (1)
Line range hint
1-88
: Documentation looks great!The Keychain SDK documentation is well-structured, comprehensive, and free of any apparent issues. It provides a clear overview of the SDK and its modules, making it easy for developers to understand and use the library.
go-client/tx_raw_client.go (1)
Line range hint
78-130
: LGTM!The changes to the
BuildTx
function introduce useful enhancements for managing gas limits:
- The new
autoEstimateGas
parameter allows the caller to specify whether gas estimation should be performed automatically.- The new
gasAdjustmentFactor
parameter allows adjusting the estimated gas based on a multiplier.- The
EstimateGas
function is called appropriately to estimate gas whenautoEstimateGas
is true.- The estimated or provided gas limit is set correctly on the transaction builder.
- The changes follow the existing code structure and style.
- Errors are handled appropriately.
Great job!
docs/developer-docs/docs/build-a-keychain/quick-start.md (2)
211-215
: LGTM!The addition of
AutoEstimateGas
andGasAdjustmentFactor
parameters enhances the flexibility and precision of gas handling. The changes are logically correct and the chosen values are reasonable.Tools
Markdownlint
213-213: Column: 1
Hard tabs(MD010, no-hard-tabs)
Line range hint
1-1
: Documentation looks good!The quick start guide provides a comprehensive and well-structured walkthrough to build a Keychain in Go using the Keychain SDK. It covers all the necessary steps with relevant code examples and explanations. No critical misspellings, grammatical errors or documentation gaps were found.
Tools
Markdownlint
213-213: Column: 1
Hard tabs(MD010, no-hard-tabs)
docs/developer-docs/docs/build-a-keychain/sdk/tutorial.md (2)
147-158
: LGTM! The addition ofAutoEstimateGas
andGasAdjustmentFactor
enhances gas management configuration.The new parameters provide more control over gas estimation and adjustment:
AutoEstimateGas
set tofalse
disables automatic gas estimation before broadcasting transactions. This allows for manual gas specification if needed.GasAdjustmentFactor
of1.2
will multiply the estimated gas by this factor. This can help ensure sufficient gas is provided, reducing the risk of out-of-gas errors.The formatting changes also improve the configuration's readability.
302-312
: Great job mirroring the app configuration in the test setup! 👍Keeping the test configuration consistent with the actual app is crucial for accurately testing the expected behavior, including the new gas management parameters.
CHANGELOG.md (1)
75-75
: LGTM!The documentation for the new feature in the
keychain-sdk
module is clear and matches the AI-generated summary. No issues found.
AutoEstimateGas bool | ||
GasAdjustmentFactor float64 |
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.
Add documentation for the new Config
fields.
The newly added AutoEstimateGas
and GasAdjustmentFactor
fields in the Config
struct are not documented. To improve clarity and usability, please add brief descriptions explaining their purpose and how they should be used.
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
docs/developer-docs/docs/build-a-keychain/implementations/keychain-sdk.md (1)
85-86
: LGTM! Please update the documentation.The new fields
AutoEstimateGas
andGasAdjustmentFactor
are appropriately named and typed. They align with the PR objective of enhancing the gas estimation process.Please update the documentation to explain these new configuration options and provide guidance on how to use them effectively.
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113
: LGTM!The addition of
AutoEstimateGas
andGasAdjustmentFactor
parameters enhances the gas management capabilities of the keychain application. SettingAutoEstimateGas
tofalse
allows for manual control over gas usage, which can be beneficial in certain scenarios. TheGasAdjustmentFactor
of1.2
provides a reasonable buffer in gas calculations, potentially improving transaction efficiency.Please replace the hard tab at the beginning of line 111 with spaces to adhere to the formatting conventions.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1 hunks)
- docs/developer-docs/docs/build-a-keychain/implementations/keychain-sdk.md (1 hunks)
- docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md (1 hunks)
Additional context used
Path-based instructions (3)
docs/developer-docs/docs/build-a-keychain/implementations/keychain-sdk.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (4)
docs/developer-docs/docs/build-a-keychain/implementations/keychain-sdk.md (1)
Line range hint
1-88
: Documentation looks good!The documentation is well-written, free of misspellings and grammatical errors. It covers all the important modules and components of the Keychain SDK, providing correct and clear explanations.
docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md (2)
52-53
: LGTM!The addition of the
AutoEstimateGas
andGasAdjustmentFactor
fields to theConfig
struct is a valuable enhancement. It allows users to configure automatic gas estimation and adjust the estimated gas by a factor, potentially improving transaction efficiency and reliability.The fields are correctly typed, annotated with
env
tags for configuration via environment variables, and have reasonable default values.
Line range hint
1-58
: Documentation looks good!The documentation for the
wardenkms.go
file is clear, detailed, and well-structured. It provides a comprehensive overview of the file's role, dependencies, configuration, main function, handlers, HTTP server, and utility functions.The explanations are concise and easy to understand, with code snippets and tables used effectively to illustrate the concepts. There are no apparent misspellings or grammatical errors in the documentation.
Great job on the documentation!
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Line range hint
1-113
: Documentation looks good!The documentation provides a clear and comprehensive guide on building a keychain application. No misspellings, grammatical errors, or missing documentation were found. The changes to the configuration parameters are well-documented in the AI-generated summary, and the documentation appears to be correct and up to date.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113
: Approve the addition of the new gas-related configuration parameters.The new
AutoEstimateGas
andGasAdjustmentFactor
parameters provide more control and flexibility over gas management in the keychain application, which is a positive change.To improve the documentation, consider adding a brief explanation of the purpose and usage of these new parameters. This will help developers understand how to configure them appropriately for their use case.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Line range hint
1-292
: Documentation looks good!I have reviewed the entire documentation file and did not find any significant spelling, grammatical, or correctness issues. The instructions are clear, and the code examples seem accurate and up-to-date. Great job on maintaining high-quality documentation!
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
i suppose this closes #681 |
Yes. |
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
keychain-sdk/example_keychain_test.go (1)
33-37
: LGTM! Consider adding explanatory comments.The new parameters
AutoEstimateGas
andGasAdjustmentFactor
have been successfully implemented, aligning with the PR objectives. The example now demonstrates automatic gas estimation with realistic values, addressing previous review comments.Consider adding brief comments explaining the purpose of
AutoEstimateGas
andGasAdjustmentFactor
for better documentation. For example:// AutoEstimateGas enables automatic gas estimation before broadcasting transactions AutoEstimateGas: true, // GasAdjustmentFactor provides a safety buffer for estimated gas (e.g., 1.2 means 20% extra) GasAdjustmentFactor: 1.2,While the current implementation with a fixed
GasAdjustmentFactor
is a good start, consider exploring dynamic gas adjustment strategies in the future. This could involve adjusting the factor based on historical gas usage data or network conditions, potentially optimizing gas costs while maintaining transaction reliability.docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113
: Consider adding brief explanations for the new gas-related parameters.The new gas-related parameters (
AutoEstimateGas
andGasAdjustmentFactor
) are excellent additions that enhance gas management. To further improve the documentation:Consider adding brief explanations for these new parameters. For example:
// setup gas management GasLimit: 400000, // Maximum gas allowed per transaction AutoEstimateGas: true, // Automatically estimate gas before broadcasting GasAdjustmentFactor: 1.2, // Adjust estimated gas by 20% to provide a bufferThis will help users understand the purpose and impact of these new configuration options.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- cmd/wardenkms/wardenkms.go (2 hunks)
- docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1 hunks)
- docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md (1 hunks)
- go-client/tx_raw_client.go (6 hunks)
- keychain-sdk/example_keychain_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- cmd/wardenkms/wardenkms.go
- docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md
- go-client/tx_raw_client.go
Additional context used
Path-based instructions (2)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"keychain-sdk/example_keychain_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Markdownlint
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (2)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (2)
109-109
: LGTM: Gas limit configuration looks good.The
GasLimit
parameter is set to a reasonable value of 400000. This configuration aligns with the existing setup mentioned in the AI summary.
110-110
: Great addition: AutoEstimateGas enables dynamic gas management.Setting
AutoEstimateGas
to true is an excellent enhancement. This feature allows for automatic gas estimation before broadcasting transactions, which can significantly improve transaction efficiency and reduce the likelihood of failures due to insufficient gas allocation.
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113
: Enhance documentation with explanations for new parameters.The documentation is comprehensive, but it would be beneficial to add brief explanations for the newly introduced parameters. Consider adding the following information after the configuration example:
- `AutoEstimateGas`: When set to `true`, it enables automatic gas estimation before broadcasting transactions. This helps in determining the appropriate gas limit for each transaction dynamically. - `GasAdjustmentFactor`: This factor (set to 1.2 in the example) is used to adjust the estimated gas. It provides a 20% buffer to the estimated gas, helping to prevent transaction failures due to underestimation.This addition will help developers understand the purpose and impact of these new configuration options.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113
: Approve the addition of AutoEstimateGas and GasAdjustmentFactor parameters.The new configuration parameters
AutoEstimateGas
andGasAdjustmentFactor
have been correctly added to the Keychain app configuration. These additions align with the PR objectives:
AutoEstimateGas: true
enables automatic gas estimation before broadcasting transactions.GasAdjustmentFactor: 1.2
provides a 20% buffer in gas calculations, which can help prevent transaction failures due to underestimation.These changes will improve the efficiency of gas management in the Keychain app.
Tools
Markdownlint
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
78-78
: LGTM with a minor suggestion for consistency.The addition of the AutoEstimateGas and GasAdjustmentFactor features is well-documented. However, for consistency with other entries, consider adding an issue number (if applicable) and slightly rewording the entry.
Consider updating the entry as follows:
-* (keychain-sdk) Add AutoEstimateGas and GasAdjustmentFactor parameters to automatically estimate gas before broadcasting tx +* (keychain-sdk) [#issue-number] Add `AutoEstimateGas` and `GasAdjustmentFactor` parameters to automatically estimate gas before broadcasting transactionsThis change improves consistency with other entries and provides more context.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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.
looks good to me, i'm leaving just two nits before we can merge this
BatchSize: 10, | ||
GasLimit: 400000, | ||
AutoEstimateGas: true, | ||
GasAdjustmentFactor: 1.2, |
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.
GasAdjustmentFactor: 1.2, | |
GasAdjustmentFactor: 1.2, |
replace tabs with spaces
// AutoEstimateGas is a flag to estimate gas before broadcasting the transaction | ||
// and use it as GasLimit. | ||
// | ||
// When AutoEstimateGas == true then GasLimit = min(EstimatedGas, GasLimit) |
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.
// When AutoEstimateGas == true then GasLimit = min(EstimatedGas, GasLimit) | |
// When AutoEstimateGas == true then GasLimit = min(EstimatedGas * GasAdjustmentFactor, GasLimit) |
Summary by CodeRabbit
New Features
AutoEstimateGas
andGasAdjustmentFactor
parameters to enhance transaction broadcasting efficiency.Description
andUrl
.Bug Fixes
Documentation