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

[x/concentratedliquidity]: Fix Incorrect Event Emission #8755

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

mattverse
Copy link
Member

Closes: #XXX

What is the purpose of the change

Minor fix on incorrect event emission

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@mattverse mattverse added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Oct 5, 2024
Copy link
Contributor

coderabbitai bot commented Oct 5, 2024

Walkthrough

The changes in this pull request focus on enhancing the Keeper struct methods within the concentrated_liquidity package. Key modifications include improved error handling and event emission for the CreatePosition, WithdrawPosition, and addToPosition methods. These updates ensure that liquidity management functions operate more robustly, particularly by addressing edge cases and refining error messages related to liquidity operations.

Changes

File Path Change Summary
x/concentrated-liquidity/lp.go - Enhanced CreatePosition: Added error handling for zero liquidity delta and refined event emission logic.
- Improved WithdrawPosition: Added checks for liquidity limits and clarified pool state handling.
- Updated addToPosition: Included checks for last position and refined event emission logic.
CHANGELOG.md - Updated changelog to organize changes by categories such as "State Breaking" and "State Compatible," detailing significant changes and linking to relevant pull requests.

Possibly related PRs

  • chore: changelog fixes #8423: This PR focuses on fixing the changelog, which is related to the documentation updates in the main PR's CHANGELOG.md.
  • misc: Add missing CHANGELOG #8686: This PR adds a missing entry in the changelog, contributing to the documentation aspect of the main PR's changes in CHANGELOG.md.

Suggested labels

V:state/compatible/backport, A:backport/v25.x

Suggested reviewers

  • PaddyMc

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c2e670e and e4639f5.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
CHANGELOG.md (8)

Line range hint 1-61: Major release v19.0.0 introduces significant changes and new features

This release includes several important updates:

  1. New modules:

    • SuperCharged Liquidity (x/concentrated-liquidity)
    • CosmWasm Pool (x/cosmwasmpool)
    • ProtoRev changes
    • TokenFactory before send hooks
  2. API breaks and state-breaking changes:

    • Multiple changes to existing APIs and state structures
    • Updates to SDK and dependencies
  3. Security updates:

    • Upgrade to wasmvm 1.2.3

These changes will likely require significant updates for node operators, developers, and users of the Osmosis ecosystem. It's crucial to carefully review the upgrade instructions and test thoroughly before applying this update to production environments.


63-65: Important security update to wasmvm

The upgrade to wasmvm 1.2.3 addresses the security vulnerability CWA-2023-002. This is a critical update that should be applied as soon as possible to mitigate potential security risks.


Line range hint 67-77: Significant new features and module enhancements

This release introduces several important features:

  1. SuperCharged Liquidity module (x/concentrated-liquidity) for enhanced capital efficiency
  2. CosmWasm Pool module (x/cosmwasmpool) for custom liquidity pools
  3. ProtoRev changes, including modifications to payment schedules and new triggers
  4. TokenFactory before send hooks for custom logic execution

These features greatly expand Osmosis's capabilities, particularly in terms of capital efficiency, customization, and integration with CosmWasm smart contracts. Developers and users should explore these new features to leverage the enhanced functionality.


Line range hint 79-93: Multiple API breaks require attention from developers

This release includes several API changes that will impact existing integrations:

  1. Modifications to queries in x/lockup, x/superfluid, and x/poolmanager
  2. Changes to message structures in x/gamm
  3. Updates to stargate query whitelists
  4. Removal of some API methods and addition of new ones

Developers need to carefully review these changes and update their applications accordingly. It's recommended to thoroughly test all integrations with the new API to ensure compatibility.


Line range hint 95-110: State-breaking changes require careful upgrade planning

This release includes several state-breaking changes:

  1. Modifications to ICA authorized messages
  2. Changes to TWAP record upgrade handlers
  3. Updates to synthetic locks and epoch distribution
  4. Adjustments to gas consumption for various operations
  5. Changes to consensus parameters

Node operators and developers need to pay close attention to these changes during the upgrade process. It's crucial to follow the upgrade instructions carefully and ensure all state migrations are performed correctly to avoid any disruptions in the network.


Line range hint 112-116: Important dependency updates

This release includes updates to key dependencies:

  1. Upgrade to wasmd 0.31
  2. Cosmwasm security patch (Cherry)
  3. IBC patch (Huckleberry)

These updates are crucial for maintaining security and compatibility with the broader Cosmos ecosystem. Ensure that your environment is compatible with these new versions when upgrading.


Line range hint 118-155: Numerous miscellaneous improvements and bug fixes

This release includes a wide range of improvements and bug fixes:

  1. Performance enhancements, including IAVL speedups and optimizations in various modules
  2. Bug fixes in areas such as supply queries, CL math operations, and event emissions
  3. CLI improvements and new commands
  4. Code refactoring and cleanup for better maintainability
  5. Additional queries and improvements to existing ones
  6. Updates to Docker configurations and build processes

These changes contribute to a more stable, efficient, and developer-friendly Osmosis ecosystem. While not as impactful as the major features and breaking changes, these improvements are valuable for the overall health of the project.


Line range hint 1-155: Comprehensive update with significant impacts across the Osmosis ecosystem

The v19.0.0 release of Osmosis represents a major update with far-reaching implications for the entire ecosystem. Key points to consider:

  1. New features like SuperCharged Liquidity and CosmWasm Pools offer enhanced functionality and customization options.
  2. Multiple API breaks and state-breaking changes require careful attention from developers and node operators.
  3. Important security updates and dependency upgrades improve the overall security and compatibility of the system.
  4. Numerous miscellaneous improvements enhance performance, stability, and developer experience.

Recommendations:

  1. Node operators should carefully follow the upgrade instructions and test thoroughly in a non-production environment before upgrading.
  2. Developers need to review all API changes and update their applications accordingly.
  3. Users should be aware of new features and potential changes in behavior, especially related to liquidity pools and token interactions.
  4. All participants in the Osmosis ecosystem should stay informed about these changes and their potential impacts on their specific use cases.

This release marks a significant step forward for Osmosis, but it requires careful consideration and preparation from all stakeholders to ensure a smooth transition.


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.

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)
x/concentrated-liquidity/lp.go (1)

Line range hint 435-452: Consider Modifying the Position Without Full Withdrawal

The current implementation of addToPosition withdraws the full position and creates a new one with the added liquidity. This approach may introduce rounding errors and is less efficient due to the overhead of withdrawing and recreating positions.

Consider refactoring the logic to add liquidity directly to the existing position without full withdrawal. This can be achieved by updating the position's liquidity and state in place, which would:

  • Reduce the potential for rounding errors caused by withdrawal and re-deposit.
  • Improve performance by minimizing state changes and event emissions.
  • Simplify the logic by eliminating the need to handle withdrawal and re-creation.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f0a195f and 924096b.

📒 Files selected for processing (1)
  • x/concentrated-liquidity/lp.go (1 hunks)
🔇 Additional comments (1)
x/concentrated-liquidity/lp.go (1)

461-461: Good Fix: Include Pool ID in Event Emission

By adding sdk.NewAttribute(types.AttributeKeyPoolId, strconv.FormatUint(pool.GetId(), 10)), the emitted event now correctly includes the poolId. This ensures that event listeners receive accurate and complete information, improving traceability and auditing of pool-related events.

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

yikes, thanks for fixing

@mattverse mattverse closed this Oct 8, 2024
@mattverse mattverse reopened this Oct 8, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Oct 17, 2024
@mattverse mattverse merged commit fb356ba into main Oct 17, 2024
1 check passed
@mattverse mattverse deleted the mattverse/keplr-events branch October 17, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity Stale V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants