Add AutoEstimateGas and GasAdjustmentFactor parameters to estimate gas automatically before broadcasting the tx #847
Conversation
…ly estimate gas before broadcasting tx
📝 Walkthrough📝 WalkthroughWalkthroughThe changes update the Keychain app configuration by adding two new gas-related parameters: Changes
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
EstimateGasfunction is a great addition that provides automatic gas estimation by simulating the transaction. It has the following merits:
- It returns the provided
gasLimitwhenautoEstimateGasis 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
gasLimitby 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
EstimateGasmore 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
EstimateGasfunction more readable by moving the simulation logic into a separatesimulateTxfunction. The main flow ofEstimateGasbecomes 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-sdkandtestifypackages are used for. This can help readers understand the purpose of these dependencies.In the "Creating the main application" section, the placeholder for the
handleKeyRequestfunction appears before the completemain.gocode. 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
AutoEstimateGastofalsedisables 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
AutoEstimateGasis set to false. TheEstimateGasfunction ingo-client/tx_raw_client.gorespects this setting and uses the providedgasLimitwhen auto-estimation is disabled. Thekeychain-sdkand 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! TheAutoEstimateGasflag is a useful addition.The
AutoEstimateGasflag provides flexibility to automatically estimate the gas required for transactions. This can help:
- Optimize transaction costs by setting the
GasLimitbased on estimated needs.- Prevent transactions from failing due to insufficient gas.
- Avoid overpaying for gas by setting an appropriate limit.
Good job!
57-61: TheGasAdjustmentFactoris a thoughtful addition. Nice work!The
GasAdjustmentFactorprovides 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
AutoEstimateGasfollows the Golang naming convention, uses the appropriate type, and has the correct visibility.
30-31: LGTM!The new field
GasAdjustmentFactorfollows the Golang naming convention, uses the appropriate type, and has the correct visibility.
145-145: LGTM!The
BuildTxmethod call is updated correctly to include the newAutoEstimateGasandGasAdjustmentFactorparameters.keychain-sdk/keychain.go (1)
126-129: LGTM! The changes enhance the transaction handling capabilities.The code segment initializes the
Fees,AutoEstimateGas, andGasAdjustmentFactorproperties of thetxWriterinstance using values from the configuration. This allows for more flexible and efficient transaction management:
- The
Feesproperty ensures that transaction fees are aligned with the configuration settings.- The
AutoEstimateGasproperty enables automatic gas estimation for transactions, which can improve efficiency and reduce the likelihood of transaction failures due to insufficient gas.- The
GasAdjustmentFactorproperty 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
Configstruct follow the Uber Golang Style Guide and provide useful configuration options for gas estimation and transaction parameters. The use ofenvtags with default values is a good practice.
62-74: Looks good!The
keychain.NewAppinstantiation 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 thesdkandmathpackages.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
BuildTxfunction introduce useful enhancements for managing gas limits:
- The new
autoEstimateGasparameter allows the caller to specify whether gas estimation should be performed automatically.- The new
gasAdjustmentFactorparameter allows adjusting the estimated gas based on a multiplier.- The
EstimateGasfunction is called appropriately to estimate gas whenautoEstimateGasis 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
AutoEstimateGasandGasAdjustmentFactorparameters 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 ofAutoEstimateGasandGasAdjustmentFactorenhances gas management configuration.The new parameters provide more control over gas estimation and adjustment:
AutoEstimateGasset tofalsedisables automatic gas estimation before broadcasting transactions. This allows for manual gas specification if needed.GasAdjustmentFactorof1.2will 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-sdkmodule is clear and matches the AI-generated summary. No issues found.
| AutoEstimateGas bool | ||
| GasAdjustmentFactor float64 |
There was a problem hiding this comment.
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.
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
AutoEstimateGasandGasAdjustmentFactorare 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
AutoEstimateGasandGasAdjustmentFactorparameters enhances the gas management capabilities of the keychain application. SettingAutoEstimateGastofalseallows for manual control over gas usage, which can be beneficial in certain scenarios. TheGasAdjustmentFactorof1.2provides 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
AutoEstimateGasandGasAdjustmentFactorfields to theConfigstruct 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
envtags for configuration via environment variables, and have reasonable default values.
Line range hint
1-58: Documentation looks good!The documentation for the
wardenkms.gofile 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.
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
AutoEstimateGasandGasAdjustmentFactorparameters 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.
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
AutoEstimateGasandGasAdjustmentFactorhave 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
AutoEstimateGasandGasAdjustmentFactorfor 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
GasAdjustmentFactoris 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 (
AutoEstimateGasandGasAdjustmentFactor) 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
GasLimitparameter 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
AutoEstimateGasto 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.
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
AutoEstimateGasandGasAdjustmentFactorhave been correctly added to the Keychain app configuration. These additions align with the PR objectives:
AutoEstimateGas: trueenables automatic gas estimation before broadcasting transactions.GasAdjustmentFactor: 1.2provides 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)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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"
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
Outdated
Show resolved
Hide resolved
| // 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.
| // When AutoEstimateGas == true then GasLimit = min(EstimatedGas, GasLimit) | |
| // When AutoEstimateGas == true then GasLimit = min(EstimatedGas * GasAdjustmentFactor, GasLimit) |
Co-authored-by: Antonio Pitasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
108-114: Add an explanatory sentence for the new gas parametersThe snippet now contains
AutoEstimateGasandGasAdjustmentFactor, but the surrounding prose still says “setup throughput for batching responses”, so readers have zero context for the new knobs. A single sentence before or after the snippet that explains that
AutoEstimateGasenables automatic gas estimation, andGasAdjustmentFactoracts as a safety multiplier around the estimatewould prevent confusion.
- // setup throughput for batching responses + // setup throughput for batching responses and automatic gas estimation(or add a short paragraph right below the code block).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Files:
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: artur-abliazimov
PR: warden-protocol/wardenprotocol#1069
File: automated-orders/packages/scheduler/src/config/config.ts:13-13
Timestamp: 2024-11-14T07:15:50.941Z
Learning: In the `automated-orders/packages/scheduler/src/config/config.ts` file, storing private keys in environment variables is currently acceptable. The team is aware of the security implications, and improvements are being tracked in issue #87192305.
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
Learnt from: artur-abliazimov
PR: #1069
File: automated-orders/packages/scheduler/src/config/config.ts:13-13
Timestamp: 2024-11-14T07:15:50.941Z
Learning: In the automated-orders/packages/scheduler/src/config/config.ts file, storing private keys in environment variables is currently acceptable. The team is aware of the security implications, and improvements are being tracked in issue #87192305.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
docs/developer-docs/docs/build-a-keychain/build-a-keychain-app.md (1)
109-113: Verify hard-tabs were removedMarkdown-lint previously flagged a hard tab on what is now Line 111. From the diff alone it’s impossible to see whether that character is still a tab or has been replaced by spaces. Please re-run
markdownlint/prettierand ensure indentation is spaces-only.
Summary by CodeRabbit
New Features
AutoEstimateGasandGasAdjustmentFactorparameters to enhance transaction broadcasting efficiency.DescriptionandUrl.Bug Fixes
Documentation