fix(vesting): delegated on addGrant #2880
Conversation
WalkthroughThe pull request introduces updates to the Evmos project's vesting account functionality. Key changes include the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
x/vesting/keeper/msg_server.go (1)
493-496: Unnecessary Error Handling forBondDenomFunctionThe
BondDenommethod typically does not return an error; it returns the bond denomination string directly. The error handling around this call is unnecessary and can be removed to simplify the code.Apply this diff to remove redundant error handling:
- bondDenom, err := k.stakingKeeper.BondDenom(ctx) - if err != nil { - return nil, err - } + bondDenom := k.stakingKeeper.BondDenom(ctx)x/vesting/keeper/msg_server_test.go (1)
113-113: Maintain consistency in test case names.The test case names on lines 113 and 124 start with
"true -"while other test cases use"pass -"or"fail -"for clarity. Consider changing"true -"to"pass -"to maintain consistency across your test case names.Apply this diff to update the test case names:
- name: "true - create vesting account with pre existing delegation", + name: "pass - create vesting account with pre existing delegation", - name: "true - fund existing vesting account with pre existing delegation", + name: "pass - fund existing vesting account with pre existing delegation",Also applies to: 124-124
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- x/vesting/keeper/msg_server.go (3 hunks)
- x/vesting/keeper/msg_server_test.go (9 hunks)
- x/vesting/types/clawback_vesting_account.go (1 hunks)
Additional comments not posted (3)
CHANGELOG.md (3)
49-49: LGTM! Comprehensive changelog for the upcoming release.The Unreleased section is well-structured and provides a clear overview of the upcoming changes. It properly categorizes the changes into State Machine Breaking, Improvements, and Bug Fixes, making it easy for users to understand the impact of the new version.
Line range hint
51-93: LGTM! Well-documented v15.0.0 release.The v15.0.0 section provides a comprehensive overview of the changes in this major release. It clearly outlines the API Breaking and State Machine Breaking changes, along with numerous improvements and bug fixes. This level of detail is crucial for users upgrading to this version.
Line range hint
95-1037: LGTM! Excellent maintenance of the project's version history.The CHANGELOG.md file maintains a consistent and detailed record of changes across all versions of the project. This comprehensive history is invaluable for users and developers to understand the project's evolution, breaking changes, and improvements over time. The consistent categorization of changes (e.g., State Machine Breaking, API Breaking, Improvements, Bug Fixes) across versions is particularly helpful.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
…into GAtom22/fix-add-grant
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/vesting/keeper/msg_server.go (2 hunks)
- x/vesting/types/clawback_vesting_account.go (3 hunks)
Additional comments not posted (3)
x/vesting/keeper/msg_server.go (3)
75-75: LGTM: Minor formatting improvementThe addition of a blank line improves code readability by separating logical blocks of code.
158-160: Reminder: Address potential issues with delegated tokensWhile the current changes simplify the code, they don't directly address the potential issues mentioned in previous reviews. These issues include:
- Potential inclusion of unvested delegated tokens in
DelegatedFree.- Incorrect assignment of delegated tokens to
DelegatedFreein theaddGrantfunction (now removed).getDelegatedCoinsnot differentiating between vested and unvested tokens.Please review these concerns and ensure that the new implementation correctly handles the assignment of vested and unvested delegated tokens to
DelegatedFreeandDelegatedVesting.To investigate these issues, please run the following verification script:
158-160: Simplification of grant addition logicThe removal of the
addGrantfunction and its replacement with a direct call tovestingAcc.AddGrantsimplifies the code and streamlines the process of modifying vesting schedules. This change appears to maintain the core functionality while reducing complexity.To ensure the new implementation maintains the same behavior as the previous one, please run the following verification script:
Verification successful
Grant addition logic successfully simplified
The removal of the
addGrantfunction and its replacement with a direct call tovestingAcc.AddGranthas been successfully verified. No remaining references to the oldaddGrantfunction were found, ensuring the change maintains the intended functionality without introducing complexity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of AddGrant in the ClawbackVestingAccount type # Test: Check the implementation of AddGrant in ClawbackVestingAccount ast-grep --lang go --pattern $'type ClawbackVestingAccount struct { $$$ } func (cva *ClawbackVestingAccount) AddGrant($_) error { $$$ }' # Test: Check for any remaining references to the old addGrant function rg 'func.*addGrant'Length of output: 696
Script:
#!/bin/bash # Description: Verify the existence of ClawbackVestingAccount struct # Test: Check the ClawbackVestingAccount struct definition ast-grep --lang go --pattern $'type ClawbackVestingAccount struct { $$$ }' # Description: Verify the implementation of AddGrant method in ClawbackVestingAccount # Test: Check the AddGrant method implementation ast-grep --lang go --pattern $'func (cva *ClawbackVestingAccount) AddGrant($_) error { $$$ }' # Test: Check for any remaining references to the old addGrant function rg 'func.*addGrant'Length of output: 7990
Script:
#!/bin/bash # Description: Identify the type of vestingAcc in msg_server.go # Locate the declaration or assignment of vestingAcc and extract its type rg 'vestingAcc\s*[:=]\s*New\w+\(' -- go:msg_server.go --replace '$1' | awk '{print $NF}' | head -n 1 # Description: Verify if the identified type implements the AddGrant method # Assuming the type identified is stored in a variable TYPE, replace <TYPE> accordingly # Replace <TYPE> with the actual type name obtained from the previous command TYPE="<TYPE>" ast-grep --lang go --pattern $"func (\\w+ *${TYPE}) AddGrant($_) error { $$$ }" # Description: Search the codebase for all implementations of AddGrant method # Find all AddGrant method implementations ast-grep --lang go --pattern $'func (\\$_ *$_) AddGrant($_) error { $$$ }'Length of output: 427
Script:
#!/bin/bash # Description: Locate all instances of msg_server.go in the codebase # Find all paths to msg_server.go msg_server_paths=$(fd --type f --name "msg_server.go") # Check if any msg_server.go files are found if [ -z "$msg_server_paths" ]; then echo "No msg_server.go files found in the repository." exit 1 fi echo "Found msg_server.go at the following locations:" echo "$msg_server_paths" # Iterate through each msg_server.go path to extract the type of vestingAcc and verify AddGrant for path in $msg_server_paths; do echo "Processing $path..." # Extract the line where vestingAcc is initialized or assigned vestingAcc_line=$(rg 'vestingAcc\s*[:=]\s*New\w+\(' "$path" | head -n 1) if [ -z "$vestingAcc_line" ]; then echo "No initialization or assignment of vestingAcc found in $path." continue fi # Extract the type name from the vestingAcc initialization type_name=$(echo "$vestingAcc_line" | grep -oP 'New\K\w+') if [ -z "$type_name" ]; then echo "Could not determine the type of vestingAcc in $path." continue fi echo "Identified vestingAcc type: $type_name" # Verify if the identified type implements the AddGrant method ast-grep --lang go --pattern $'func (cva *'"$type_name"'") AddGrant($_) error { $$$ }' "$path" # Search the codebase for all implementations of AddGrant method ast-grep --lang go --pattern $'func ($_ *$_) AddGrant($_) error { $$$ }' done # Finally, check for any remaining references to the old addGrant function rg 'func.*addGrant'Length of output: 436
Script:
#!/bin/bash # Description: Locate all instances of msg_server.go in the codebase using rg # Find all paths to msg_server.go using ripgrep msg_server_paths=$(rg --files --glob 'msg_server.go') # Check if any msg_server.go files are found if [ -z "$msg_server_paths" ]; then echo "No msg_server.go files found in the repository." exit 1 fi echo "Found msg_server.go at the following locations:" echo "$msg_server_paths" # Iterate through each msg_server.go path to extract the type of vestingAcc and verify AddGrant for path in $msg_server_paths; do echo "Processing $path..." # Extract the line where vestingAcc is initialized or assigned vestingAcc_line=$(rg 'vestingAcc\s*[:=]\s*New\w+\(' "$path" | head -n 1) if [ -z "$vestingAcc_line" ]; then echo "No initialization or assignment of vestingAcc found in $path." continue fi # Extract the type name from the vestingAcc initialization type_name=$(echo "$vestingAcc_line" | grep -oP 'New\K\w+') if [ -z "$type_name" ]; then echo "Could not determine the type of vestingAcc in $path." continue fi echo "Identified vestingAcc type: $type_name" # Verify if the identified type implements the AddGrant method ast-grep --lang go --pattern $'func (cva *'"$type_name"'") AddGrant($_) error { $$$ }' "$path" # Search the codebase for all implementations of AddGrant method ast-grep --lang go --pattern $'func ($_ *$_) AddGrant($_) error { $$$ }' done # Finally, check for any remaining references to the old addGrant function rg 'func.*addGrant'Length of output: 4215
There was a problem hiding this comment.
Thanks for addressing this @GAtom22! Only left some small suggestions for personal preference but feel free to ignore them
|
https://github.com/Mergifyio backport rama/release-v20 |
✅ Backports have been created
|
* fix(vesting): delegated on creation and addGrant * add changelog entry * update comments * update comments * Update x/vesting/keeper/msg_server_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * Update x/vesting/keeper/msg_server_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * update comments * remove init value for DelegatedFree * move addGrant to clawback acc type * update chlog * address review comments --------- Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 1d66cbc) # Conflicts: # CHANGELOG.md # x/vesting/keeper/msg_server.go # x/vesting/keeper/msg_server_test.go
* fix(vesting): delegated on addGrant (#2880) * fix(vesting): delegated on creation and addGrant * add changelog entry * update comments * update comments * Update x/vesting/keeper/msg_server_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * Update x/vesting/keeper/msg_server_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> * update comments * remove init value for DelegatedFree * move addGrant to clawback acc type * update chlog * address review comments --------- Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 1d66cbc) # Conflicts: # CHANGELOG.md # x/vesting/keeper/msg_server.go # x/vesting/keeper/msg_server_test.go * fix conflicts * fix conflicts * run make format --------- Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com> Co-authored-by: GAtom22 <GAtom22@users.noreply.github.com>
Description
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...
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...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit
New Features
AddGrantmethod for merging clawback vesting grants into existing accounts.Bug Fixes
Tests