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

refactor: remove panic usage in keeper methods #18636

Merged
merged 19 commits into from
Dec 12, 2023
Merged

refactor: remove panic usage in keeper methods #18636

merged 19 commits into from
Dec 12, 2023

Conversation

likhita-809
Copy link
Contributor

@likhita-809 likhita-809 commented Dec 6, 2023

Description

ref: #15555

This PR removes the panic usage in keeper methods from all(x/*) modules.


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...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Copy link
Contributor

coderabbitai bot commented Dec 6, 2023

Walkthrough

The Cosmos SDK has undergone a significant update focused on improving error handling across various modules. Panics have been replaced with error returns to ensure more robust and graceful error management. This change affects methods in modules like x/bank, x/distribution, x/slashing, and x/staking. Additionally, the client/keys module has been enhanced to display mnemonics discreetly, with an option to disable this feature. The types module also sees the addition of a new function.

Changes

File(s) Change Summary
x/bank/keeper/keeper.go Replaced panics with error returns in coin transfer and account management functions.
x/staking/keeper/delegation.go, x/staking/keeper/slash.go Improved error handling by replacing panics with error returns in delegation and slashing functions.
x/distribution/keeper/delegation.go, x/distribution/keeper/grpc_query.go Replaced panics with error returns and added error handling for slash event iteration.
x/slashing/keeper/signing_info.go Modified JailUntil and Tombstone functions to return errors instead of panicking when signing info is missing.
x/staking/keeper/alias_functions.go, x/staking/keeper/keeper_test.go Updated serialization mechanism and error handling in keeper functions and tests.
x/gov/keeper/.../common_test.go, .../deposit_test.go, .../grpc_query_test.go, .../hooks_test.go, .../keeper_test.go, .../msg_server_test.go, .../proposal_test.go, .../tally_test.go Introduced new import for gogoproto/proto, modified TestProposal variable handling, and used require.ErrorContains for error checking in tests.
client/keys Enhanced mnemonic display with discreet screen and added --indiscreet option.
types Added AmountOfNoValidation function to sdk.DecCoins.

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 X ?


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

@likhita-809 likhita-809 changed the title refactor(x/staking): remove panic usage in keeper methods refactor: remove panic usage in keeper methods Dec 6, 2023
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @likhita-809 for taking this on 👏

@likhita-809 likhita-809 marked this pull request as ready for review December 8, 2023 08:49
@likhita-809 likhita-809 requested a review from a team as a code owner December 8, 2023 08:49
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: 10

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f6df368 and c1ac1d2.
Files selected for processing (20)
  • x/bank/keeper/keeper.go (4 hunks)
  • x/bank/keeper/keeper_test.go (3 hunks)
  • x/distribution/keeper/delegation.go (4 hunks)
  • x/distribution/keeper/grpc_query.go (1 hunks)
  • x/gov/keeper/common_test.go (2 hunks)
  • x/gov/keeper/deposit_test.go (4 hunks)
  • x/gov/keeper/grpc_query_test.go (17 hunks)
  • x/gov/keeper/hooks_test.go (2 hunks)
  • x/gov/keeper/keeper_test.go (3 hunks)
  • x/gov/keeper/msg_server_test.go (2 hunks)
  • x/gov/keeper/proposal_test.go (5 hunks)
  • x/gov/keeper/tally_test.go (2 hunks)
  • x/gov/keeper/vote_test.go (2 hunks)
  • x/slashing/keeper/signing_info.go (2 hunks)
  • x/staking/keeper/alias_functions.go (2 hunks)
  • x/staking/keeper/delegation.go (6 hunks)
  • x/staking/keeper/keeper_test.go (2 hunks)
  • x/staking/keeper/slash.go (5 hunks)
  • x/staking/keeper/val_state_change.go (5 hunks)
  • x/staking/keeper/validator.go (4 hunks)
Additional comments: 71
x/bank/keeper/keeper.go (7)
  • 257-267: The changes in the SendCoinsFromModuleToAccount function correctly replace panic calls with error returns, enhancing error handling as per the PR objectives. The function now returns an error if the module account does not exist or if the recipient address is black-listed.

  • 275-289: The SendCoinsFromModuleToModule function has been updated to return an error if either module account does not exist, aligning with the PR's goal to improve error handling by avoiding panic calls.

  • 293-302: The SendCoinsFromAccountToModule function now returns an error if the module account does not exist, which is consistent with the PR's objective to enhance error handling.

  • 305-320: The DelegateCoinsFromAccountToModule function's error handling has been improved to return an error if the module account does not exist or is unauthorized, which is in line with the PR's aim to replace panic calls with error returns.

  • 323-338: The UndelegateCoinsFromModuleToAccount function has been refactored to return an error if the module account does not exist or is unauthorized, adhering to the PR's objectives.

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

The MintCoins function now returns an error if the module account does not exist or is unauthorized, and includes a check for minting permissions, which aligns with the PR's goal of improving error handling.

  • 381-385: The BurnCoins function has been updated to return an error if the account does not exist or is unauthorized to burn tokens, which is consistent with the PR's objective to enhance error handling.
x/bank/keeper/keeper_test.go (4)
  • 396-412: The test TestSupply_DelegateUndelegateCoins is correctly checking the error scenarios when sending coins from a module to an account with an empty string as the module name, which should result in an error as the module account does not exist. The test also checks for sending coins between module accounts and from a module to a base account, ensuring that the correct errors are returned when the module account is not found. The test then proceeds to check the balance transfers and delegation logic, which seems to be correctly implemented.

  • 453-469: The test TestSupply_SendCoins is similar to the previous one, TestSupply_DelegateUndelegateCoins, and it also correctly checks for error scenarios when sending coins between module accounts and from a module to a base account. It ensures that the correct errors are returned when the module account is not found and validates the balance transfers.

  • 502-522: The test TestSupply_MintCoins is checking the minting functionality. It ensures that an error is returned when trying to mint coins to a non-existent module account and when the module account does not have the permission to mint tokens. It also checks for negative amounts, which should result in an error. The test then proceeds to mint coins to a valid module account with the correct permissions and checks that no error is returned. This test is correctly validating the minting process and permissions.

  • 396-412: The tests TestSupply_DelegateUndelegateCoins, TestSupply_SendCoins, and TestSupply_MintCoins are correctly implemented to check various error scenarios and validate the correct behavior of the bank module's functionalities. They ensure that the module accounts exist and have the necessary permissions before performing actions like sending or minting coins. The tests also validate that the balances are updated correctly after these operations.

Also applies to: 453-469, 502-522

x/distribution/keeper/delegation.go (5)
  • 51-65: The error handling for k.IterateValidatorSlashEventsBetween is correctly implemented, aligning with the PR's objective to improve error handling by replacing panic calls with error returns.

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

The error handling within the IterateValidatorSlashEventsBetween function is well implemented, with proper error propagation and handling of iteration errors.

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

The sanity checks added to calculateDelegationRewardsBetween function are good for ensuring that the input parameters are valid, which is in line with the PR's objective to improve robustness.

  • 77-83: The check for negative rewards is a good addition for ensuring the correctness of the rewards calculation logic.

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

The code does not show any panic calls being replaced with error returns in the provided hunks, which is mentioned in the PR objective and the summary of changes. If the panic replacements are in other parts of the file not shown in the hunks, please ensure they are consistent with the PR's objective.

x/distribution/keeper/grpc_query.go (1)
  • 264-305: The refactoring of the iteration process to capture and return errors instead of panicking is consistent with the PR's objective to improve error handling. The implementation correctly sets iterErr upon encountering an error and returns it after the iteration, ensuring that errors are propagated as suggested in the previous review comment.
x/gov/keeper/common_test.go (2)
  • 33-37: The initialization of TestProposal ignores the error returned by getTestProposal. Confirm that this is intentional and that the error can never occur or its impact is properly considered.

  • 45-56: The changes to getTestProposal function correctly replace panic with error returns, aligning with the PR's objective to improve error handling.

x/gov/keeper/deposit_test.go (4)
  • 54-60: The refactoring of the tp variable from a single TestProposal to a slice of proto.Message is correctly implemented with a nil check to ensure backward compatibility.

  • 7-7: The addition of the github.com/cosmos/gogoproto/proto import is appropriate for the usage of proto.Message in the test cases.

  • 232-238: The changes to the tp variable declaration are consistently applied in the TestDepositAmount function as well.

  • 425-431: The changes to the tp variable declaration are consistently applied in the TestChargeDeposit function as well.

x/gov/keeper/grpc_query_test.go (7)
  • 446-451: The introduction of the tp variable and its conditional assignment from TestProposal is consistent with the summary provided. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 447-451: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 566-570: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 684-689: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 792-797: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 1289-1293: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

  • 1389-1394: The introduction of the tp variable and its conditional assignment from TestProposal is not mentioned in the summary. This change should be verified to ensure that TestProposal is correctly replaced by tp in all relevant function calls.

x/gov/keeper/hooks_test.go (2)
  • 5-11: The addition of the github.com/cosmos/gogoproto/proto import is consistent with the changes described in the PR objectives and the summary.

  • 78-84: The change to declare tp as a slice of proto.Message and the conditional assignment based on TestProposal being non-nil aligns with the PR objectives to refactor the code and improve error handling.

x/gov/keeper/keeper_test.go (3)
  • 3-9: The addition of the github.com/cosmos/gogoproto/proto import is consistent with the PR objectives and the summary provided.

  • 89-98: The handling of TestProposal as a slice of proto.Message in TestIncrementProposalNumber aligns with the PR objectives and the summary provided.

  • 119-128: The handling of TestProposal as a slice of proto.Message in TestProposalQueues aligns with the PR objectives and the summary provided.

x/gov/keeper/msg_server_test.go (2)
  • 4-11: The addition of the import statement for github.com/cosmos/gogoproto/proto is confirmed and correctly placed in the import block.

  • 1786-1791: The modification in the TestSubmitProposal_InitialDeposit function, which introduces a conditional check and assignment for the tp variable before creating a new message using v1.NewMsgSubmitProposal, is confirmed and correctly implemented.

x/gov/keeper/proposal_test.go (6)
  • 6-12: The import of github.com/cosmos/gogoproto/proto is consistent with the changes made to handle TestProposal as a slice of proto.Message.

  • 32-38: The conditional check for TestProposal being non-nil before assignment to tp and subsequent use in SubmitProposal is a good practice to prevent potential nil pointer dereferences.

  • 61-70: The pattern of checking TestProposal for nil and using it in SubmitProposal is consistently applied across test cases, which is good for maintainability.

  • 90-97: The same pattern of handling TestProposal is correctly applied in the TestActivateVotingPeriod function.

  • 123-131: The handling of TestProposal as a slice and the nil check before its use in SubmitProposal within the TestDeleteProposalInVotingPeriod function is consistent with the rest of the changes.

  • 61-70: While the PR aims to replace panic calls with error returns, the use of suite.Require().NotPanics in test cases is still valid as it asserts that the function under test does not panic, which is a legitimate test assertion.

x/gov/keeper/tally_test.go (3)
  • 4-10: The addition of the github.com/cosmos/gogoproto/proto import is consistent with the changes made in the code.

  • 397-399: The summary incorrectly states that TestProposal is assigned to tp. The code actually shows that tp is conditionally assigned the value of TestProposal if it is not nil.

  • 403-403: The change in the function invocation from using TestProposal to tp is consistent with the summary provided.

x/gov/keeper/vote_test.go (4)
  • 3-9: The addition of the github.com/cosmos/gogoproto/proto import is consistent with the changes made in the test function.

  • 24-27: The change to tp being a slice of proto.Message and its conditional assignment is consistent with the PR objectives to handle TestProposal as a slice.

  • 28-28: The modification to the govKeeper.SubmitProposal function call to include tp as an argument and an empty string for the second argument is consistent with the PR objectives.

  • 24-27: Verify that the change of TestProposal from a single value to a slice of proto.Message is correctly reflected in all relevant parts of the codebase.


The verification process has confirmed that the variable TestProposal is now treated as a slice of proto.Message across various test files in the codebase. The change is consistent with the new context provided.

Final review comment:

The change of TestProposal from a single value to a slice of proto.Message is correctly reflected in all relevant parts of the codebase.

x/staking/keeper/alias_functions.go (1)
  • 65-65: The comment // XXX is this safe will the validator unexposed fields be able to get written to? indicates a potential safety concern. It is important to ensure that the validator's unexposed fields are not being inadvertently modified during the iteration. This may require a deeper review of the fn function passed as an argument and the Validator type's methods to ensure encapsulation is not broken.
x/staking/keeper/delegation.go (2)
  • 219-225: The summary mentions replacing panic calls with return statements, but the provided code does not show any such changes. It's possible that the summary is outdated or incorrect. Please verify the accuracy of the summary against the current state of the code.

  • 219-225: The summary indicates changes to the function signatures of GetDelegatorBonded, Delegate, Unbond, and getBeginInfo to include error returns, but the provided code does not reflect these changes. Please verify the accuracy of the summary against the current state of the code.

x/staking/keeper/keeper_test.go (2)
  • 356-362: The change from specific marshaling functions to the generic MustMarshal method of the codec instance is consistent with the PR's objective to refactor serialization mechanisms. This should simplify the code and potentially make it more maintainable.

  • 444-450: The marshaling and setting of the val object in the store using the MustMarshal method and Set function are correctly implemented. This aligns with the PR's objective to refactor serialization mechanisms.

x/staking/keeper/slash.go (5)
  • 57-63: The error logging for a nonexistent validator is well-handled, ensuring that the system does not fail silently.

  • 191-195: The error handling for invalid validator status is correctly implemented, ensuring that the function returns an error for any undefined validator status.

  • 212-219: The error handling in the Jail method is correctly implemented, providing a clear error message when the validator is not found.

  • 227-234: The error handling in the Unjail method is correctly implemented, providing a clear error message when the validator is not found.

  • 369-375: The error handling for unknown validator status in the SlashRedelegation method is correctly implemented, ensuring that the function returns an error for any undefined validator status.

x/staking/keeper/val_state_change.go (6)
  • 156-166: The error handling in ApplyAndReturnValidatorSetUpdates has been updated to return errors instead of panicking, which improves the robustness of the code by allowing for better error handling and recovery.

  • 161-161: The error message correctly formats the validator address in hexadecimal using %X.

  • 165-165: The error handling for jailed validators in the power store is correctly implemented, ensuring that such a critical invariant violation is caught and reported.

  • 191-191: The default case in the switch statement provides a safety net for unhandled validator statuses, which is a good practice for defensive programming.

  • 281-302: The state transition functions bondedToUnbonding, unbondingToBonded, and unbondedToBonded have been refactored to return errors instead of panicking, which aligns with the PR's goal of improving error handling.

  • 396-397: The sanity check in BeginUnbondingValidator ensures that the validator is in the expected state before beginning the unbonding process, which is a good practice to prevent incorrect state transitions.

x/staking/keeper/validator.go (5)
  • 299-308: The code correctly handles the case where a validator record is not found by returning an error, aligning with the PR's objective to improve error handling by replacing panic calls with error returns.

  • 34-39: The function GetValidatorByConsAddr correctly returns an error if the consensus address is not found, which is consistent with the PR's objective to replace panic calls with error returns.

  • 48-53: The function SetValidator correctly handles errors from the StringToBytes conversion, which is a good practice for robust error handling.

  • 368-374: The function GetLastValidators correctly handles the case where the number of validators exceeds the maximum allowed, returning an error instead of panicking, which is in line with the PR's objective.

  • 299-308: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [34-374]

Verify that the changes made to the functions GetBondedValidatorsByPower and GetLastValidators are consistent with the rest of the codebase and do not introduce any breaking changes.


The search results indicate that the modified functions GetBondedValidatorsByPower and GetLastValidators are used in various test files within the x/staking/keeper package and in integration tests. The presence of err in the function calls suggests that the updated functions, which now return an error, are being handled correctly in the test files. There are no indications of misuse or unhandled errors in the provided context.

Final review comment:

The verification process confirms that the updated functions GetBondedValidatorsByPower and GetLastValidators are correctly utilized throughout the codebase, with proper error handling in place as per the changes made in the PR. No issues were found in the test files where these functions are invoked.

x/slashing/keeper/signing_info.go Show resolved Hide resolved
x/slashing/keeper/signing_info.go Show resolved Hide resolved
x/staking/keeper/alias_functions.go Outdated Show resolved Hide resolved
x/distribution/keeper/delegation.go Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
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 c1ac1d2 and b7bd994.
Files selected for processing (4)
  • x/distribution/keeper/delegation.go (5 hunks)
  • x/slashing/keeper/signing_info.go (2 hunks)
  • x/staking/keeper/alias_functions.go (2 hunks)
  • x/staking/keeper/delegation.go (7 hunks)
Files skipped from review as they are similar to previous changes (3)
  • x/distribution/keeper/delegation.go
  • x/slashing/keeper/signing_info.go
  • x/staking/keeper/alias_functions.go
Additional comments: 7
x/staking/keeper/delegation.go (7)
  • 216-227: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [216-237]

The summary mentions changes to function signatures, but the provided hunks do not show any changes to function signatures for GetDelegatorBonded. This might indicate an inconsistency between the summary and the actual changes.

  • 722-728: The error message for the check on tokenSrc being bonded is generic. Consider providing a more descriptive error message to aid in debugging.
- 725:         return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded")
+ 725:         return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded; expected Unbonded or Unbonding, got Bonded")
  • 216-227: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [216-237]

The refactoring of GetDelegatorBonded to use a separate iterErr variable for capturing iteration errors is a good practice for robust error handling.

  • 722-728: The error message for the check on tokenSrc being bonded is generic. Consider providing a more descriptive error message to aid in debugging.
- 725:         return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded")
+ 725:         return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded; expected Unbonded or Unbonding, got Bonded")
  • 722-728: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [722-770]

The refactoring of Delegate to handle different token source statuses and transfer tokens between pools if necessary is a good practice for robust error handling.

  • 837-847: The refactoring of Unbond to jail a validator if undelegating will decrease the validator's self-delegation below their minimum is a good practice for robust error handling.

  • 914-919: The refactoring of getBeginInfo to handle different validator statuses and return a more informative error message when the status is unknown is a good practice for robust error handling.

@julienrbrt julienrbrt self-assigned this Dec 8, 2023
x/gov/keeper/common_test.go Outdated Show resolved Hide resolved
x/gov/keeper/deposit_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

overall looks good. if we changed the function signature to return we should add this to the changelog file to help teams like cosmwasm

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! Can we get a changelog about the behavior 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 b7bd994 and 6685fb6.
Files selected for processing (1)
  • x/gov/keeper/tally_test.go (1 hunks)
Additional comments: 1
x/gov/keeper/tally_test.go (1)
  • 396-396: The addition of a blank line at 396 seems to be a formatting change and does not impact the logic or flow of the code. It's typically good practice to keep formatting consistent, but this change is minor and does not affect the functionality.

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: 5

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6685fb6 and e77cbc4.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Additional comments: 2
CHANGELOG.md (2)
  • 56-62: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [27-27]

Deprecation of a feature.

  • 56-62: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [29-29]

Removal of a feature.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@likhita-809
Copy link
Contributor Author

can we merge this @julienrbrt ?

@julienrbrt
Copy link
Member

can we merge this @julienrbrt ?

I think so yeah. However, we should update the changelog to be exhaustive. It isn't clear which methods have a new behavior currently.

@likhita-809
Copy link
Contributor Author

likhita-809 commented Dec 11, 2023

I think so yeah. However, we should update the changelog to be exhaustive. It isn't clear which methods have a new behavior currently.

updated changelog

x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
x/staking/keeper/delegation.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@julienrbrt julienrbrt added this pull request to the merge queue Dec 12, 2023
Merged via the queue into main with commit 38f40c4 Dec 12, 2023
63 of 65 checks passed
@julienrbrt julienrbrt deleted the likhita/15555 branch December 12, 2023 09:53
marcello33 added a commit to 0xPolygon/cosmos-sdk that referenced this pull request Dec 13, 2023
* feat: secp256k1 public key constant time (cosmos#18026)

Signed-off-by: bizk <[email protected]>

* chore: Fixed changelog duplicated items (cosmos#18628)

* adr: Un-Ordered Transaction Inclusion (cosmos#18553)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* docs: lint ADR-070 (cosmos#18634)

* fix(baseapp)!: postHandler should run regardless of result (cosmos#18627)

* docs: fix typos in adr-007-specialization-groups.md (cosmos#18635)

* chore: alphabetize labels (cosmos#18640)

* docs(x/circuit): add note on ante handler (cosmos#18637)

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* fix: telemetry metric label variable (cosmos#18643)

* chore: typos fix (cosmos#18642)

* refactor(store/v2): updates from integration (cosmos#18633)

* build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

* feat(store/v2): snapshot manager (cosmos#18458)

* chore(client/v2): fix typos in the README.md (cosmos#18657)

* fix(baseapp):  protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654)

Co-authored-by: unknown unknown <unknown@unknown>

* chore: fix several minor typos (cosmos#18660)

* chore(tools/confix/cmd): fix typo in view.go (cosmos#18659)

* refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655)

* feat(accounts): use gogoproto API instead of protov2.  (cosmos#18653)

Co-authored-by: unknown unknown <unknown@unknown>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651)

* build(deps): Bump actions/stale from 8 to 9 (cosmos#18656)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(docs): fix typos & wording in docs (cosmos#18667)

* chore: fix several typos.   (cosmos#18666)

* feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Marko <[email protected]>

* feat(store/v2): add SetInitialVersion in SC (cosmos#18665)

* feat(client/keys): support display discreetly for `keys add` (cosmos#18663)

Co-authored-by: Julien Robert <[email protected]>

* ci: add misspell action (cosmos#18671)

* chore: typos fix by misspell-fixer (cosmos#18683)

Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Julien Robert <[email protected]>

* chore: add v0.50.2 changelog to main (cosmos#18682)

* build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* refactor(bank): remove .String() calls  (cosmos#18175)

Co-authored-by: Facundo <[email protected]>

* ci: use codespell instead of misspell-fixer (cosmos#18686)

Co-authored-by: Marko <[email protected]>

* feat(gov): add proposal types and spam votes (cosmos#18532)

* feat(accounts): use account number as state prefix for account state (cosmos#18664)

Co-authored-by: unknown unknown <unknown@unknown>

* chore: typos fixes by cosmos-sdk bot (cosmos#18689)

Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: marbar3778 <[email protected]>

* feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688)

* refactor: remove panic usage in keeper methods (cosmos#18636)

* ci: rename pr name in misspell job (cosmos#18693)

Co-authored-by: Marko <[email protected]>

* build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

* feat(client/keys): support display discreetly for keys export (cosmos#18684)

* feat(x/gov): better gov genesis validation (cosmos#18707)

---------

Signed-off-by: bizk <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Carlos Santiago Yanzon <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: Akaonetwo <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: dreamweaverxyz <[email protected]>
Co-authored-by: Pioua <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: cool-developer <[email protected]>
Co-authored-by: leonarddt05 <[email protected]>
Co-authored-by: testinginprod <[email protected]>
Co-authored-by: unknown unknown <unknown@unknown>
Co-authored-by: Sukey <[email protected]>
Co-authored-by: axie <[email protected]>
Co-authored-by: Luke Ma <[email protected]>
Co-authored-by: Emmanuel T Odeke <[email protected]>
Co-authored-by: 0xn4de <[email protected]>
Co-authored-by: hattizai <[email protected]>
Co-authored-by: Devon Bear <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Halimao <[email protected]>
Co-authored-by: Cosmos SDK <[email protected]>
Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Facundo <[email protected]>
Co-authored-by: Likhita Polavarapu <[email protected]>
@marcello33 marcello33 mentioned this pull request Jan 22, 2024
12 tasks
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.

5 participants