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

Add AutoEstimateGas and GasAdjustmentFactor parameters to estimate gas automatically before broadcasting the tx #847

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

artur-abliazimov
Copy link
Contributor

@artur-abliazimov artur-abliazimov commented Sep 16, 2024

Summary by CodeRabbit

  • New Features

    • Introduced AutoEstimateGas and GasAdjustmentFactor parameters to enhance transaction broadcasting efficiency.
    • Improved configurability for gas management during transactions.
    • Added support for new metadata fields in the Keychain, including Description and Url.
  • Bug Fixes

    • Resolved an issue with default parameters in the GMP configuration.
  • Documentation

    • Updated developer documentation to reflect new configuration parameters and their usage.

Copy link
Contributor

coderabbitai bot commented Sep 16, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes introduce two new parameters, AutoEstimateGas and GasAdjustmentFactor, to the keychain-sdk module and related components. These parameters enhance the transaction broadcasting process by allowing automatic gas estimation and providing a mechanism to adjust gas estimates dynamically. Additionally, modifications were made to various configuration files and structures to support these new features across multiple modules.

Changes

File Path Change Summary
keychain-sdk/... Added AutoEstimateGas and GasAdjustmentFactor to Config struct.
cmd/wardenkms/... Introduced AutoEstimateGas and GasAdjustmentFactor in Config struct; modified keychain.NewApp to include new parameters.
go-client/... Added parameters to BuildTx and introduced EstimateGas helper function for gas estimation.
keychain-sdk/keychain.go Initialized txWriter with new properties: AutoEstimateGas and GasAdjustmentFactor.
docs/developer-docs/docs/build-a-keychain/sdk/keychain-sdk.md Documented new fields AutoEstimateGas and GasAdjustmentFactor in Config struct.
docs/developer-docs/docs/build-a-keychain/implementations/wardenkms.md Updated Config struct to include new fields for gas management.
keychain-sdk/internal/writer/writer.go Added AutoEstimateGas and GasAdjustmentFactor fields to W struct and modified sendWaitTx method.
keychain-sdk/example_keychain_test.go Updated configuration structure to include new parameters for gas management.

Possibly related PRs

  • Make tx fees configurable in keychain-sdk #281: Introduces configurable transaction fees in the keychain-sdk, which relates to the changes in the main PR that also involve enhancements to transaction handling and fee management.
  • fix(keychain-sdk): use ethsecp256k1 instead of cosmos secp256k1 #805: Changes the cryptographic library used in the go-client from Cosmos secp256k1 to ethsecp256k1, aligning with the main PR's updates regarding the handling of keychain fees and transaction broadcasting.
  • rm template_id fields #840: Removes the template_id fields from various components, which may relate to the overall cleanup and restructuring of transaction handling in the main PR.
  • update evmos to v20 #919: Updates the Evmos version to v20, relevant as the main PR also includes updates to dependencies and enhancements in transaction management.
  • update ethermint proto & wardenjs #921: Updates the Ethermint proto and WardenJS, relevant to the changes in the main PR that involve updates to the keychain and transaction handling.

Suggested reviewers

  • Pitasi

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 when autoEstimateGas 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 separate simulateTx function. The main flow of EstimateGas 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:

  1. In the "Setting up the project" section, consider adding a brief explanation of what the keychain-sdk and testify packages are used for. This can help readers understand the purpose of these dependencies.

  2. In the "Creating the main application" section, the placeholder for the handleKeyRequest function appears before the complete main.go code. Consider moving it after the complete code snippet to maintain a logical flow.

  3. 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

Commits

Files that changed from the base of the PR and between 1206dea and 4475b4b.

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 to false 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. The EstimateGas function in go-client/tx_raw_client.go respects this setting and uses the provided gasLimit when auto-estimation is disabled. The keychain-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! The AutoEstimateGas 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: The GasAdjustmentFactor 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 new AutoEstimateGas and GasAdjustmentFactor parameters.

keychain-sdk/keychain.go (1)

126-129: LGTM! The changes enhance the transaction handling capabilities.

The code segment initializes the Fees, AutoEstimateGas, and GasAdjustmentFactor properties of the txWriter 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 of env 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 the sdk and math 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 when autoEstimateGas 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 and GasAdjustmentFactor 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 of AutoEstimateGas and GasAdjustmentFactor enhances gas management configuration.

The new parameters provide more control over gas estimation and adjustment:

  • AutoEstimateGas set to false disables automatic gas estimation before broadcasting transactions. This allows for manual gas specification if needed.
  • GasAdjustmentFactor of 1.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.

keychain-sdk/example_keychain_test.go Show resolved Hide resolved
keychain-sdk/internal/writer/writer.go Show resolved Hide resolved
Comment on lines 85 to 86
AutoEstimateGas bool
GasAdjustmentFactor float64
Copy link
Contributor

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.

Copy link

vercel bot commented Sep 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
warden-help-center ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2024 7:09am

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and GasAdjustmentFactor 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 and GasAdjustmentFactor parameters enhances the gas management capabilities of the keychain application. Setting AutoEstimateGas to false allows for manual control over gas usage, which can be beneficial in certain scenarios. The GasAdjustmentFactor of 1.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

Commits

Files that changed from the base of the PR and between d5967b3 and b86a8ce.

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 and GasAdjustmentFactor fields to the Config 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)

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and GasAdjustmentFactor 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

Commits

Files that changed from the base of the PR and between b86a8ce and 7947983.

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)

@Pitasi
Copy link
Contributor

Pitasi commented Sep 23, 2024

i suppose this closes #681

@artur-abliazimov
Copy link
Contributor Author

i suppose this closes #681

Yes.

go-client/tx_raw_client.go Outdated Show resolved Hide resolved
go-client/tx_raw_client.go Show resolved Hide resolved
go-client/tx_raw_client.go Outdated Show resolved Hide resolved
keychain-sdk/example_keychain_test.go Outdated Show resolved Hide resolved
cmd/wardenkms/wardenkms.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and GasAdjustmentFactor 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 and GasAdjustmentFactor 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 and GasAdjustmentFactor) 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 buffer

This 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

Commits

Files that changed from the base of the PR and between 7947983 and 3eab82c.

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 3eab82c and 94ac515.

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 and GasAdjustmentFactor have been correctly added to the Keychain app configuration. These additions align with the PR objectives:

  1. AutoEstimateGas: true enables automatic gas estimation before broadcasting transactions.
  2. 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)

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 transactions

This change improves consistency with other entries and provides more context.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 94ac515 and 55fe849.

📒 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"

Copy link
Contributor

@Pitasi Pitasi left a 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// When AutoEstimateGas == true then GasLimit = min(EstimatedGas, GasLimit)
// When AutoEstimateGas == true then GasLimit = min(EstimatedGas * GasAdjustmentFactor, GasLimit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants