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

fix: missing swap event for cw pool #7938

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 3, 2024

Closes: #XXX

What is the purpose of the change

Numia team has a problem tracking swap events for a CW pool. The reason is that the event is missing.

Context: https://osmosis-network.slack.com/archives/C04M8FQ5Y2G/p1712020788660349

While there is a workaround that they can implement, it would be a hack looking for events at another layer of abstraction.

I propose adding this change to the v24 scope. The core of the change is emitting the swap event in x/cosmwasmpool.

I evaluated the option of emitting this at the poolmanager layer. However, given that we have many swap APIs, it would take some careful effort to make sure that nothing is missed. Given the above, it is cleaner and less risky to simply add the event at the x/cosmwasmpool level.

Given that events are now state breaking, it is important to account for the above risks.

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

Summary by CodeRabbit

  • New Features
    • Introduced missing swap events for CW pools to enhance transparency and tracking of swap activities.
  • Refactor
    • Revised swap event emission logic to occur at the pool module layer, improving modularity and clarity in event handling across different pool modules.
  • Documentation
    • Added comments to clarify the new approach for emitting swap events, ensuring better understanding and implementation in future developments.

@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/concentrated-liquidity C:x/poolmanager labels Apr 3, 2024
@p0mvn p0mvn added A:backport/v24.x backport patches to v24.x branch A:no-changelog and removed C:x/gamm Changes, features and bugs related to the gamm module. C:x/concentrated-liquidity C:x/poolmanager A:no-changelog labels Apr 3, 2024
@p0mvn p0mvn added the V:state/breaking State machine breaking PR label Apr 3, 2024
@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/concentrated-liquidity C:x/poolmanager labels Apr 3, 2024
@p0mvn p0mvn requested a review from PaddyMc April 3, 2024 00:05
@p0mvn p0mvn marked this pull request as ready for review April 3, 2024 00:10
Copy link
Contributor

coderabbitai bot commented Apr 3, 2024

Walkthrough

The recent updates focus on enhancing swap event reporting in CW pools by ensuring that these events are emitted at the pool module level rather than the pool manager module. This adjustment allows for more accurate and specific event tracking across different pool modules, improving transparency and functionality. The changes span several files, introducing and modifying the event emission process to align with the new approach.

Changes

File Path Change Summary
CHANGELOG.md Added feature to include missing swap events for CW pools.
x/concentrated-liquidity/swaps.go
x/gamm/keeper/swap.go
x/poolmanager/events/emit.go
Revised swap event emission to the pool module layer; added clarifications for event emission.
x/cosmwasmpool/pool_module.go Imported events from poolmanager and added swap event emission within swap functions.

🐇✨
In the realm of code, where changes abound,
A rabbit hopped in, with a soft thudding sound.
"Swap events," it whispered, "shall now be found,
At the pool's own layer, they're profoundly bound."
With a twitch of its nose, and a glance so profound,
It vanished in bytes, leaving wisdom profound.
🌟🐾

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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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/coderabbit-overrides.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.

@sunnya97
Copy link
Collaborator

sunnya97 commented Apr 3, 2024

I think this is fine for now, to avoid unexpected consequences of moving the event to poolmanager. But I do think this should be moved to the poolmanager in the future, as that seems like the right spot for it.

If we could get this into v24, that would be great

@czarcas7ic czarcas7ic merged commit c99b1c7 into main Apr 3, 2024
1 check passed
@czarcas7ic czarcas7ic deleted the roman/cwpool-swap-event branch April 3, 2024 20:22
mergify bot pushed a commit that referenced this pull request Apr 3, 2024
* fix: missing swap event for cw pool

* changelog

(cherry picked from commit c99b1c7)
czarcas7ic pushed a commit that referenced this pull request Apr 3, 2024
* fix: missing swap event for cw pool

* changelog

(cherry picked from commit c99b1c7)

Co-authored-by: Roman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v24.x backport patches to v24.x branch C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants