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

test(perp-keeper): add more tests for msg server for DnR #1686

Merged
merged 4 commits into from
Dec 1, 2023

Conversation

matthiasmatt
Copy link
Contributor

@matthiasmatt matthiasmatt commented Nov 30, 2023

Description

What does this PR do?

Purpose

Closes #1681

Summary by CodeRabbit

  • Bug Fixes

    • Implemented error handling for cases where input messages are nil during rebate allocation and withdrawal processes.
  • Tests

    • Expanded test coverage for margin addition and position closing.
    • Added new tests for the allocation and withdrawal of epoch rebates, including scenarios with nil messages and insufficient funds.
  • Documentation

    • Updated the CHANGELOG to reflect the addition of new tests and error handling improvements in the perpetual module message server.

@matthiasmatt matthiasmatt requested a review from a team as a code owner November 30, 2023 13:59
Copy link
Contributor

coderabbitai bot commented Nov 30, 2023

Warning

Rate Limit Exceeded

@Unique-Divine has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 5 minutes and 25 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. 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 0a2c1c8 and 9d8503a.

Walkthrough

The updates to the msgServer structure in the codebase involve adding nil checks for message parameters to prevent errors during the allocation and withdrawal of epoch rebates. Additionally, the test suite has been expanded with new cases to ensure the correct handling of partial closes and rebate operations, improving the robustness of the code.

Changes

File Path Change Summary
x/perp/v2/keeper/msg_server.go Added nil checks for msg parameters in AllocateEpochRebates and WithdrawEpochRebates methods.
x/perp/v2/keeper/msg_server_test.go 1. Imported inflationtypes. 2. Renamed and added test cases in TestMsgServerAddMargin. 3. Added nil message error handling in TestFailMsgServer. 4. Created TestAllocateEpochRebates for testing rebate allocation and withdrawal.
CHANGELOG.md Added a new pull request #1686 which expands test coverage for the perp module, specifically targeting the message server functionality for DnR.

Related issues (Beta)

  • NibiruChain/nibiru#1681: The PR seems to address the need for additional tests on MsgPartialClose, MsgWithdrawEpochRebates, and MsgAllocateEpochRebates as highlighted in the issue, by adding new test cases and error handling paths.

Poem

In the code where logic weaves, 🐇💻
A rabbit tests with graceful leaps.
No nil message shall pass unseen,
For CodeRabbit ensures the code is clean. 🌟✨


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

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

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 2d59c9d and 482cf72.
Files selected for processing (2)
  • x/perp/v2/keeper/msg_server.go (2 hunks)
  • x/perp/v2/keeper/msg_server_test.go (5 hunks)
Additional comments: 8
x/perp/v2/keeper/msg_server.go (3)
  • 161-168: The nil check for msg in AllocateEpochRebates method is correctly implemented to prevent nil pointer dereferences.

  • 178-185: The nil check for msg in WithdrawEpochRebates method is correctly implemented to prevent nil pointer dereferences.

  • 159-168: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [159-185]

No further issues found in the provided hunks. The changes align with the summary and improve the robustness of the code by handling potential nil pointer dereferences.

x/perp/v2/keeper/msg_server_test.go (5)
  • 7-10: The addition of the inflationtypes import is correctly reflected in the hunk and is consistent with the summary.

  • 148-148: The test case at line 148 is named "msg server close", which is consistent with the summary stating that a test case was renamed to this. However, the previous name "partial close" is not visible in the hunk, so we cannot confirm the renaming action.

  • 172-195: The new test case for "partial close" starts at line 172 and is correctly implemented, but the line numbers do not match the summary, which states lines 167-176. The actual test case extends to line 195.

  • 366-367: The summary mentions the addition of error handling for a nil message at line 344 in the TestFailMsgServer function, but this is not visible in the provided hunks. The closest related code is at lines 376-377, which checks for a nil message in a different function.

  • 444-496: The new function TestAllocateEpochRebates is correctly implemented starting at line 444, but the line numbers do not match the summary, which states line 412. The function includes tests for various scenarios, including nil message, insufficient funds, successful rebate allocation, and withdrawal of rebates for specific epochs.

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 482cf72 and 0a2c1c8.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
  • 74-75: The changelog correctly reflects the addition of new tests for the perp module msg server for DnR as described in the summary.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #1686 (5b6655a) into master (2d59c9d) will increase coverage by 0.27%.
Report is 2 commits behind head on master.
The diff coverage is 76.00%.

❗ Current head 5b6655a differs from pull request most recent head 9d8503a. Consider uploading reports for the commit 9d8503a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1686      +/-   ##
==========================================
+ Coverage   73.92%   74.19%   +0.27%     
==========================================
  Files         192      193       +1     
  Lines       15398    15445      +47     
==========================================
+ Hits        11383    11460      +77     
+ Misses       3355     3324      -31     
- Partials      660      661       +1     
Files Coverage Δ
app/app.go 58.06% <100.00%> (+0.45%) ⬆️
app/keepers.go 99.21% <100.00%> (+<0.01%) ⬆️
x/perp/v2/keeper/hooks.go 80.43% <100.00%> (+0.43%) ⬆️
x/perp/v2/keeper/keeper.go 97.46% <100.00%> (+0.13%) ⬆️
x/perp/v2/keeper/msg_server.go 100.00% <100.00%> (+27.06%) ⬆️
x/perp/v2/module/genesis.go 95.86% <100.00%> (+0.08%) ⬆️
x/perp/v2/keeper/dnr.go 84.41% <66.66%> (+2.72%) ⬆️
app/upgrades.go 55.55% <55.55%> (ø)

@Unique-Divine Unique-Divine changed the title tests: add more tests for msg server for DnR test(perp/keeper): add more tests for msg server for DnR Dec 1, 2023
@Unique-Divine Unique-Divine changed the title test(perp/keeper): add more tests for msg server for DnR test(perp-keeper): add more tests for msg server for DnR Dec 1, 2023
@Unique-Divine Unique-Divine enabled auto-merge (squash) December 1, 2023 08:12
@Unique-Divine Unique-Divine merged commit de0675a into master Dec 1, 2023
14 checks passed
@Unique-Divine Unique-Divine deleted the mat/add-more-test branch December 1, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test(perp): Logic paths for MsgPartialClose, MsgWithdrawEpochRebates, and MsgAllocateEpochRebates need tests
2 participants