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.

Sign up for GitHub

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

Jump to bottom

Speedup CL pool hooks, by only JSON marshalling if there is ahook #7685

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Speedup CL pool hooks, by only JSON marshalling if there is ahook #7685

merged 5 commits into from
Mar 12, 2024

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Mar 7, 2024 •
edited by coderabbitai bot
Loading

Closes: #7680

What is the purpose of the change

See linked issue! TL;DR saves JSON marshalling times when its not needed.

Testing and Verifying

This change is already covered by existing tests.

Documentation and Release Note

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

Summary by CodeRabbit

  • New Features
    • Introduced an IBC wasm light clients module to enhance blockchain interoperability.
  • Performance Improvements
    • Optimized marshalling processes to speed up Continuous Integration (CI) actions.
  • Refactor
    • Modified message building process in concentrated liquidity functions to use a msgBuilderFn for improved efficiency and maintainability.
    • Streamlined the message marshaling logic for contract function calls, enhancing code readability and performance.

@ValarDragon ValarDragon added the V:state/breaking State machine breaking PR label Mar 7, 2024
mattverse
mattverse approved these changes Mar 8, 2024
View reviewed changes
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM

x/concentrated-liquidity/pool_hooks.go
msgBz, err := json.Marshal(types.BeforeCreatePositionSudoMsg{BeforeCreatePosition: msg})
if err != nil {
return err
msgBuilderFn := func(poolId uint64) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that this was getting called every time! Good catch :)

@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2024 •
edited
Loading

Walkthrough

The recent updates focus on enhancing performance and efficiency within the concentrated liquidity module. By introducing a msgBuilderFn function parameter for dynamic message construction and optimizing marshalling processes, these changes aim to reduce CPU time and streamline operations. This approach minimizes unnecessary computations, especially in the absence of registered pool hooks, aligning with the objective of improving the system's overall performance.

Changes

File Path Change Summary
CHANGELOG.md Added IBC wasm light clients module (#7503) and optimized CL actions marshalling (#7685).
x/concentrated-liquidity/export_test.go
x/concentrated-liquidity/pool_hooks.go
x/concentrated-liquidity/pool_hooks_test.go		 Modified to use a msgBuilderFn for message building instead of direct marshalling.

Assessment against linked issues

Objective Addressed Explanation
Refactor CL-pool hooks to only build msgs / JSON marshal if there is a pool hook set (#7680) ✅

Poem

🐰🎉

In the land of code, where the rabbits dwell,
Changes abound, in the marshalling well.
With a hop and a skip, efficiency we chase,
Through the msgBuilderFn, we set the pace.

🚀 For performance, we yearn,
🌟 In every byte, we learn.

With each pull request, our goals align,
In the codebase vast, improvements shine.
🐰💻

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-tests 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 tests 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 tests.
    • @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.
  • The JSON schema for the configuration file is available here.
  • 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

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai[bot]
coderabbitai bot reviewed Mar 12, 2024
View reviewed changes
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.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c6c2449 and abbd6fe.
Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • x/concentrated-liquidity/export_test.go (1 hunks)
  • x/concentrated-liquidity/pool_hooks.go (3 hunks)
  • x/concentrated-liquidity/pool_hooks_test.go (1 hunks)
Additional comments: 6
x/concentrated-liquidity/pool_hooks.go (3)
  • 16-17: The introduction of msgBuilderFn type is a significant improvement in terms of performance optimization. By deferring the message construction and marshalling to only when necessary, it reduces unnecessary computational overhead. This change aligns well with the PR's objective to optimize the handling of Concentrated Liquidity pool hooks.
  • 20-24: The implementation of msgBuilderFn within BeforeCreatePosition is correctly done. It ensures that message marshalling only occurs if a CL pool hook is set for the action. This approach is consistent across all modified functions in this file, which is good for maintainability and readability.
  • 106-116: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [97-113]

The callPoolActionListener function has been refactored to accept a msgBuilderFn and construct the message only if a contract address is found for the given pool ID and action prefix. This is a crucial part of the optimization, ensuring that unnecessary message construction and marshalling are avoided. The error handling and gas metering logic within this function appear to be correctly implemented.

x/concentrated-liquidity/pool_hooks_test.go (1)
  • 170-174: The test implementation for msgBuilderFn within TestCallPoolActionListener correctly simulates the marshalling process for a test message. This ensures that the callPoolActionListener function's new behavior is adequately tested. It's good to see that the test suite has been updated to reflect the changes in the main implementation.
x/concentrated-liquidity/export_test.go (1)
  • 339-340: The modification of the CallPoolActionListener function in the test export file to accept a msgBuilderFn is consistent with the changes made in the main implementation file. This ensures that the test environment can accurately reflect and test the behavior of the main codebase. It's crucial for maintaining the integrity of the test suite.
CHANGELOG.md (1)
  • 62-62: The optimization for CL actions by only marshalling for CL hooks if they will be used, as mentioned in pull request Speedup CL pool hooks, by only JSON marshalling if there is ahook #7685, is a significant performance improvement. It's crucial to ensure that all dependent functionalities and tests are updated to accommodate this change, considering its impact on the system's efficiency.

coderabbitai[bot]
coderabbitai bot reviewed Mar 12, 2024
View reviewed changes
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between abbd6fe and 3a91876.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

@ValarDragon ValarDragon merged commit 401c589 into main Mar 12, 2024
1 check passed
@ValarDragon ValarDragon deleted the dev/speedup_pool_hook_json_marshal branch March 12, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pipello Pipello Pipello left review comments

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@mattverse mattverse mattverse approved these changes

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic is a code owner

@p0mvn p0mvn Awaiting requested review from p0mvn p0mvn is a code owner

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu is a code owner

Assignees
No one assigned
Labels
C:x/concentrated-liquidity V:state/breaking State machine breaking PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[perf]: Refactor CL-pool hooks to only build msgs / JSON marshal if there is a pool hook set
3 participants
@ValarDragon @Pipello @mattverse