10000 feat(app): add feemarket wrapper. Change basefee to dec by ramacarlucho · Pull Request #2878 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(app): add feemarket wrapper. Change basefee to dec #2878

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

Merged
merged 43 commits into from
Oct 2, 2024

Conversation

ramacarlucho
Copy link
Contributor
@ramacarlucho ramacarlucho commented Sep 24, 2024

Description

Add changes from #2777

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the correct branch (see PR Targeting)

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

Summary by CodeRabbit

  • New Features

    • Introduced a governance precompile to enhance transaction handling and governance capabilities.
    • Added a new function for converting amounts to an 18-decimal representation, improving EVM coin handling.
  • Bug Fixes

    • Adjusted method signatures for improved functionality, including gas estimation and delegation management.
    • Updated return type for base fee query to enhance representation.
    • Changed data types for base fee and gas price fields to improve precision in calculations.
  • Documentation

    • Updated changelog to reflect significant updates and changes across multiple versions.
    • Added documentation comments to clarify the purpose of the chain configuration.
  • Tests

    • Enhanced test coverage for EVM coin conversions and updated configuration methods for EVM coin information.
    • Introduced new tests to validate adjustments in decimal handling.
    • Expanded test suite for querying Ethereum accounts and related functionalities, including new test cases for various scenarios.
  • Chores

    • Bumped dependencies to newer versions for improved compatibility and performance.

Copy link
Contributor
coderabbitai bot commented Sep 24, 2024

Warning

Rate limit exceeded

@ramacarlucho has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 7 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 8cceca5 and 444f2bd.

Walkthrough

The changes across multiple files enhance the Evmos project, focusing on updates in account management, transaction handling, and fee management. Key modifications include the removal of the EthAccount type in favor of BaseAccount, the introduction of a governance precompile, and improvements to gas estimation processes. A new function for converting amounts to an 18-decimal format has been added, and existing test configurations have been updated for better clarity and structure.

Changes

File Path Change Summary
CHANGELOG.md Updated to reflect changes including dependency bumps, feature enhancements, and account management updates.
x/evm/wrappers/utils.go Introduced ConvertAmountTo18DecimalsLegacy function for converting amounts to 18 decimals; modified existing functions for clarity.
x/evm/types/scaling.go Added ConvertAmountTo18DecimalsLegacy function for 18-decimal conversion; updated imports.
x/evm/keeper/config.go Updated EVMConfig and VMConfig methods to simplify base fee retrieval and parameter handling.
x/evm/keeper/keeper.go Transitioned from feeMarketKeeper to feeMarketWrapper for fee management; simplified method signatures.
x/evm/keeper/grpc_query.go Updated base fee retrieval methods to use feeMarketWrapper; removed unnecessary parameters.
x/evm/keeper/grpc_query_test.go Enhanced test cases for querying accounts and gas estimation; added new test functions and assertions.
x/evm/keeper/utils_test.go Updated test methods to standardize base fee retrieval from the EVM client; modified transaction methods.
x/evm/types/denom_config.go Added ConversionFactor method to Decimals type; restructured code for clarity.
.github/workflows/proto.yml Minor formatting adjustments in the workflow configuration; no functional changes.

Possibly related PRs

Suggested reviewers

  • hanchon
  • fedekunze
  • GAtom22
  • 0xstepit

🐇 In the meadow where the coins do gleam,
New changes hop in, like a bright sunbeam.
With decimals dancing and accounts anew,
The Evmos project shines, with a vibrant hue!
Let's cheer for the tweaks, in joy we unite,
For every transaction, the future is bright! 🌼


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.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

Signed-off-by: stepit <48993133+0xstepit@users.noreply.github.com>
Copy link
Contributor
@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

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

Great job @ramacarlucho !!!

@0xstepit 0xstepit self-requested a review September 27, 2024 10:46
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)
x/evm/types/scaling.go (2)

28-34: LGTM: Function logic is correct. Consider improving documentation.

The logic of ConvertAmountTo18DecimalsLegacy is correct and aligns with the file's purpose. However, to improve maintainability and clarity:

  1. Add a more detailed comment explaining why this function uses LegacyDec and how it differs from other conversion functions.
  2. Consider adding an example usage in the function's documentation.

Here's a suggested improvement for the function documentation:

// ConvertAmountTo18DecimalsLegacy converts the given amount from its original
// decimal representation to 18 decimals using LegacyDec for backwards compatibility.
// This function is used in legacy parts of the system that still rely on LegacyDec.
//
// Example usage:
//
//	originalAmount := sdkmath.LegacyNewDec(1000000) // Assuming 6 decimals
//	convertedAmount := ConvertAmountTo18DecimalsLegacy
8000
(originalAmount)
//	// convertedAmount will be 1000000000000000000 (18 decimals)
func ConvertAmountTo18DecimalsLegacy(amt sdkmath.LegacyDec) sdkmath.LegacyDec {
	// ... (existing function body)
}
🧰 Tools
🪛 GitHub Check: test-unit-cover

[failure] 31-31:
undefined: evmtypes


[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: Run golangci-lint

[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)

🪛 GitHub Check: ante-benchmark-test

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: test-solidity

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: dependency-review

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: build

[failure] 31-31:
undefined: evmtypes


28-34: Document the rationale and future plans for legacy decimal handling.

The addition of ConvertAmountTo18DecimalsLegacy using sdkmath.LegacyDec suggests a transition period or backwards compatibility requirement. To improve long-term maintainability and clarity:

  1. Add a comment at the package level or in a central documentation file explaining the rationale for introducing this legacy function.
  2. Document any plans for future updates to decimal handling in the system.
  3. If applicable, create a tracking issue for eventually migrating away from LegacyDec usage.

Would you like assistance in drafting this documentation or creating a tracking issue?

🧰 Tools
🪛 GitHub Check: test-unit-cover

[failure] 31-31:
undefined: evmtypes


[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: Run golangci-lint

[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)

🪛 GitHub Check: ante-benchmark-test

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: test-solidity

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: dependency-review

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: build

[failure] 31-31:
undefined: evmtypes

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 084a85a and 8cceca5.

📒 Files selected for processing (1)
  • x/evm/types/scaling.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: test-unit-cover
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes


[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: Run golangci-lint
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)


[failure] 31-31:
undefined: evmtypes) (typecheck)

🪛 GitHub Check: ante-benchmark-test
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: test-solidity
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: dependency-review
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes

🪛 GitHub Check: build
x/evm/types/scaling.go

[failure] 31-31:
undefined: evmtypes

🔇 Additional comments (1)
x/evm/types/scaling.go (1)

10-10: LGTM: Import addition is correct and necessary.

The addition of sdkmath "cosmossdk.io/math" is appropriate for the new function's use of sdkmath.LegacyDec.

@ramacarlucho ramacarlucho merged commit 09efa6a into main Oct 2, 2024
52 of 54 checks passed
@ramacarlucho ramacarlucho deleted the rama/feemerket-wrapper branch October 2, 2024 14:56
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.

5 participants
0