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: fetch CometInfo from service #20238

Merged
merged 14 commits into from
May 3, 2024
Merged

refactor: fetch CometInfo from service #20238

merged 14 commits into from
May 3, 2024

Conversation

kocubinski
Copy link
Member

@kocubinski kocubinski commented May 1, 2024

Description

An alternative to #19602, and required for server v2 work, this PR:

  • Provides an implementation of CometInfoService which fetches from context (in server v2 this may be a different source, possibly a keeper)
  • Supplies the service to Environment
  • Refactors 3 usages of CometInfo fetching to use it instead

As a big plus this removes the dependency on the SDK from x/consensus.

closes #19599


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

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Enhanced context-aware information retrieval service introduced for improved data access and manipulation.
  • Refactor
    • Modules updated to utilize the new service for fetching Comet information, ensuring efficient data handling.
  • Documentation
    • Documentation updated to reflect new service integrations and function changes across modules.

Copy link
Contributor

coderabbitai bot commented May 1, 2024

Walkthrough

Walkthrough

The recent updates across various modules in the cosmos-sdk project introduce the core/comet.Service, enhancing the method of accessing Comet and ABCI information. This global change standardizes the retrieval process of critical blockchain data, ensuring a more context-aware and efficient system architecture.

Changes

File(s) Summary of Changes
core/appmodule/v2/environment.go, runtime/comet.go, runtime/environment.go Added core/comet.Service and implemented it in new and existing structures for improved context-aware data handling.
x/evidence/keeper/abci.go, x/slashing/abci.go, x/staking/keeper/historical_info.go Refactored data access to utilize core/comet.Service instead of direct SDK context calls, enhancing modularity and maintainability.
x/evidence/depinject.go, x/slashing/depinject.go, x/staking/depinject.go Updated dependency injection to include core/comet.Service, aligning with the new service-oriented architecture.
x/evidence/module.go, x/slashing/module.go, runtime/module.go Modified module structures and initialization to integrate core/comet.Service, facilitating centralized and efficient data access across different modules.
simapp/app.go Included comet.Service in app initialization, ensuring all relevant modules are equipped with the new service for data access.

Assessment against linked issues

Objective Addressed Explanation
Add consensus module messages to slashing and evidence for Comet-related information (Issue #19599)

Recent Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between f896734 and bb5368f.
Files selected for processing (3)
  • x/evidence/CHANGELOG.md (1 hunks)
  • x/slashing/CHANGELOG.md (1 hunks)
  • x/staking/CHANGELOG.md (1 hunks)
Additional Context Used
LanguageTool (196)
x/evidence/CHANGELOG.md (44)

Near line 28: Possible spelling mistake found.
Context: ... --> # Changelog ## [Unreleased] ### Api Breaking Changes * [#20238](https://gi...


Near line 30: Unpaired symbol: ‘[’ seems to be missing
Context: ...ed] ### Api Breaking Changes * [#20238](https://github.com/cosmos/cosmos-sdk/pu...


Near line 30: Possible spelling mistake found.
Context: ...ithub.com//pull/20238) NewAppModule now takes in a core/comet.Service an...


Near line 30: Add a space between sentences.
Context: ...ll/20238) NewAppModule now takes in a core/comet.Service an argument. BeginBlocker now takes...


Near line 30: Possible spelling mistake found.
Context: ...in a core/comet.Service an argument. BeginBlocker now takes in a core/comet.Service. *...


Near line 30: Add a space between sentences.
Context: ...rgument. BeginBlocker now takes in a core/comet.Service. * [#20016](https://github.com/cosmos/...


Near line 31: Unpaired symbol: ‘[’ seems to be missing
Context: ...kes in a core/comet.Service. * [#20016](https://github.com/cosmos/cosmos-sdk/pu...


Near line 31: Possible spelling mistake found.
Context: ...ithub.com//pull/20016) NewMsgSubmitEvidence now takes a string as argument instead...


Near line 31: Possible spelling mistake found.
Context: ...akes a string as argument instead of an AccAddress. * [#19482](https://github.com/cosmos/...


Near line 32: Unpaired symbol: ‘[’ seems to be missing
Context: ...nt instead of an AccAddress. * [#19482](https://github.com/cosmos/cosmos-sdk/pu...


Near line 32: Add a space between sentences.
Context: ...ithub.com//pull/19482) appmodule.Environment is passed to NewKeeper instead of in...


Near line 32: Possible spelling mistake found.
Context: ...2) appmodule.Environment is passed to NewKeeper instead of individual services * [#196...


Near line 33: Unpaired symbol: ‘[’ seems to be missing
Context: ...instead of individual services * [#19627](https://github.com/cosmos/cosmos-sdk/pu...


Near line 33: Possible spelling mistake found.
Context: ...ithub.com//pull/19627) NewAppModule now takes in a codec.Codec as its fi...


Near line 33: Add a space between sentences.
Context: ...ll/19627) NewAppModule now takes in a codec.Codec as its first argument ## [v0.1.1](ht...


Near line 36: Unpaired symbol: ‘[’ seems to be missing
Context: ...odec` as its first argument ## [v0.1.1](https://github.com/cosmos/cosmos-sdk/re...


Near line 43: Unpaired symbol: ‘[’ seems to be missing
Context: ...smossdk.io/store` to v1.1.0. ## [v0.1.0](https://github.com/cosmos/cosmos-sdk/re...


Near line 47: Unpaired symbol: ‘[’ seems to be missing
Context: ....0) - 2023-11-07 ### Features * [14724](https://github.com/cosmos/cosmos-sdk/pu...


Near line 47: If a new sentence starts here, add a space and start with an uppercase letter.
Context: ...dule is extracted to have a separate go.mod file which allows it be a standalone mo...


Near line 47: The preposition ‘to’ may be missing (allow someone to do something).
Context: ... a separate go.mod file which allows it be a standalone module. * (keeper) [#15420...


Near line 48: Possible spelling mistake found.
Context: ....com//pull/15420) Move BeginBlocker to the keeper folder & make HandleEqui...


Near line 48: Possible spelling mistake found.
Context: ...ginBlocker` to the keeper folder & make HandleEquivocation private ### API Breaking Changes * [#...


Near line 52: Unpaired symbol: ‘[’ seems to be missing
Context: ...ate ### API Breaking Changes * [#16008](https://github.com/cosmos/cosmos-sdk/pu...


Near line 52: Possible spelling mistake found.
Context: ...ithub.com//pull/16008) NewKeeper now takes in a KVStoreService instead o...


Near line 52: Possible spelling mistake found.
Context: ...dk/pull/16008) NewKeeper now takes in a KVStoreService instead of KVStoreKey, most functions u...


Near line 52: Possible spelling mistake found.
Context: ...ow takes in a KVStoreService instead of KVStoreKey, most functions use context.Context ins...


Near line 52: Add a space between sentences.
Context: ... KVStoreKey, most functions use context.Context instead of sdk.Context and `IterateEvid...


Near line 52: Possible spelling mistake found.
Context: ...unctions use context.Context instead of sdk.Context and IterateEvidence callback ...


Near line 52: Add a space between sentences.
Context: ...ions use context.Context instead of sdk.Context and IterateEvidence callback function...


Near line 52: Possible spelling mistake found.
Context: ...text.Context instead of sdk.Context and IterateEvidence callback function now returns an error...


Near line 52: Add a space between sentences.
Context: ...now returns an error to stop iteration (errors.ErrStopIterating). * (keeper) [#15825](https://github.c...


Near line 53: Add a space between sentences.
Context: ...5) Evidence constructor now requires an address.Codec (import "cosmossdk.io/core/address")...


Near line 54: Unpaired symbol: ‘[’ seems to be missing
Context: ... "cosmossdk.io/core/address"`) * [#16336](https://github.com/cosmos/cosmos-sdk/pu...


Near line 55: Possible spelling mistake found.
Context: ...state management: * Removed: keeper SetEvidence, GetEvidence, IterateEvidences, `G...


Near line 55: Possible spelling mistake found.
Context: ...t: * Removed: keeper SetEvidence, GetEvidence, IterateEvidences, GetAllEvidences...


Near line 55: Possible spelling mistake found.
Context: ...d: keeper SetEvidence, GetEvidence, IterateEvidences, GetAllEvidences, `MustMarshalEviden...


Near line 55: Possible spelling mistake found.
Context: ...ce, GetEvidence, IterateEvidences, GetAllEvidences, MustMarshalEvidence, MustUnmarshal...


Near line 55: Possible spelling mistake found.
Context: ... IterateEvidences, GetAllEvidences, MustMarshalEvidence, MustUnmarshalEvidence, `MarshalEvid...


Near line 55: Possible spelling mistake found.
Context: ...etAllEvidences, MustMarshalEvidence, MustUnmarshalEvidence, MarshalEvidence, UnmarshalEvidence...


Near line 55: Possible spelling mistake found.
Context: ...shalEvidence, MustUnmarshalEvidence, MarshalEvidence, UnmarshalEvidence` ### Client Break...


Near line 59: Unpaired symbol: ‘[’ seems to be missing
Context: ... ### Client Breaking Changes * [#16625](https://github.com/cosmos/cosmos-sdk/pu...


Near line 59: Possible typo: you repeated a word
Context: ...b.com//pull/16625) The simd q evidence evidence command supports only querying an evid...


Near line 59: ‘an’ may be redundant when used with the uncountable noun ‘evidence’.
Context: ...vidence` command supports only querying an evidence by hash. For querying all evidences, us...


Near line 59: Possible spelling mistake found.
Context: ...y hash. For querying all evidences, use simd q evidence list instead.

x/slashing/CHANGELOG.md (36)

Near line 32: Unpaired symbol: ‘[’ seems to be missing
Context: ...## Features ### Improvements * [#19458](https://github.com/cosmos/cosmos-sdk/pu...


Near line 32: Possible spelling mistake found.
Context: ...os/cosmos-sdk/pull/19458) Avoid writing SignInfo's for validator's who did not miss a bl...


Near line 32: The possessive apostrophe seems to be incorrect. Did you mean the plural noun “validators”?
Context: ...ull/19458) Avoid writing SignInfo's for validator's who did not miss a block. (Every BeginB...


Near line 32: Possible spelling mistake found.
Context: ...ator's who did not miss a block. (Every BeginBlock) * [#18959](https://github.com/cosmos/c...


Near line 33: Unpaired symbol: ‘[’ seems to be missing
Context: ...ss a block. (Every BeginBlock) * [#18959](https://github.com/cosmos/cosmos-sdk/pu...


Near line 33: Possible spelling mistake found.
Context: ...com//pull/18959) Avoid deserialization of parameters with every validator look...


Near line 34: Unpaired symbol: ‘[’ seems to be missing
Context: ...rs with every validator lookup * [#18636](https://github.com/cosmos/cosmos-sdk/pu...


Near line 34: Possible spelling mistake found.
Context: ...ithub.com//pull/18636) JailUntil and Tombstone methods no longer pani...


Near line 38: Unpaired symbol: ‘[’ seems to be missing
Context: ...or. ### API Breaking Changes * [#20238](https://github.com/cosmos/cosmos-sdk/pu...


Near line 38: Possible spelling mistake found.
Context: ...ithub.com//pull/20238) NewAppModule now takes in a core/comet.Service an...


Near line 38: Add a space between sentences.
Context: ...ll/20238) NewAppModule now takes in a core/comet.Service an argument. BeginBlocker now takes...


Near line 38: Possible spelling mistake found.
Context: ...in a core/comet.Service an argument. BeginBlocker now takes in a core/comet.Service. *...


Near line 38: Add a space between sentences.
Context: ...rgument. BeginBlocker now takes in a core/comet.Service. * [#20026](https://github.com/cosmos/...


Near line 39: Unpaired symbol: ‘[’ seems to be missing
Context: ...kes in a core/comet.Service. * [#20026](https://github.com/cosmos/cosmos-sdk/pu...


Near line 39: Add a space between sentences.
Context: ...-sdk/pull/20026) Removal of the Address.String() method and related changes: * `Mi...


Near line 40: Possible spelling mistake found.
Context: ...ed changes: * Migrate now takes a ValidatorAddressCodec as argument. * Migrator has a ne...


Near line 41: This sentence does not start with an uppercase letter.
Context: ...essCodecas argument. *Migratorhas a new field ofValidatorAddressCodec` ...


Near line 41: Possible spelling mistake found.
Context: ...nt. * Migrator has a new field of ValidatorAddressCodec type. * [#16441](https://github.com/co...


Near line 42: Unpaired symbol: ‘[’ seems to be missing
Context: ... ValidatorAddressCodec type. * [#16441](https://github.com/cosmos/cosmos-sdk/pu...


Near line 42: Possible spelling mistake found.
Context: ...ithub.com//pull/16441) Params state is migrated to collections. `GetP...


Near line 42: Possible spelling mistake found.
Context: ...arams state is migrated to collections. GetParams has been removed. * [#17023](https://g...


Near line 43: Unpaired symbol: ‘[’ seems to be missing
Context: ... GetParams has been removed. * [#17023](https://github.com/cosmos/cosmos-sdk/pu...


Near line 43: Possible spelling mistake found.
Context: ...mos-sdk/pull/17023) Use collections for ValidatorSigningInfo: * remove Keeper: `SetValidatorS...


Near line 44: Possible spelling mistake found.
Context: ...torSigningInfo: * remove Keeper: SetValidatorSigningInfo, GetValidatorSigningInfo, IterateVa...


Near line 44: Possible spelling mistake found.
Context: ...ve Keeper: SetValidatorSigningInfo, GetValidatorSigningInfo, IterateValidatorSigningInfos * [#17...


Near line 45: Unpaired symbol: ‘[’ seems to be missing
Context: ...IterateValidatorSigningInfos * [#17044](https://github.com/cosmos/cosmos-sdk/pu...


Near line 45: Possible spelling mistake found.
Context: ...mos-sdk/pull/17044) Use collections for AddrPubkeyRelation: * remove from types: `AddrPubke...


Near line 48: Unpaired symbol: ‘[’ seems to be missing
Context: ...ove from Keeper: AddPubkey * [#19440](https://github.com/cosmos/cosmos-sdk/pu...


Near line 48: Add a space between sentences.
Context: ...l/19440) Slashing Module creation takes appmodule.Environment instead of individual services * [#194...


Near line 49: Unpaired symbol: ‘[’ seems to be missing
Context: ...instead of individual services * [#19458](https://github.com/cosmos/cosmos-sdk/pu...


Near line 49: Possible spelling mistake found.
Context: ...ithub.com//pull/19458) ValidatorSigningInfo.IndexOffset is deprecated, and no longe...


Near line 49: Add a space between sentences.
Context: ...os-sdk/pull/19458) ValidatorSigningInfo.IndexOffset is deprecated, and no longer used. The ...


Near line 49: Possible spelling mistake found.
Context: ...The index is now derived using just the StartHeight. * [#19740](https://github.com/cosmos/c...


Near line 50: Unpaired symbol: ‘[’ seems to be missing
Context: ...ed using just the StartHeight. * [#19740](https://github.com/cosmos/cosmos-sdk/pu...


Near line 50: Possible spelling mistake found.
Context: ...ithub.com//pull/19740) InitGenesis and ExportGenesis module code and ke...


Near line 50: Possible spelling mistake found.
Context: ...osmos-sdk/pull/19740) InitGenesis and ExportGenesis module code and keeper code do not pan...

x/staking/CHANGELOG.md (116)

Near line 30: Unpaired symbol: ‘[’ seems to be missing
Context: ...## [Unreleased] ### Features * [#19537](https://github.com/cosmos/cosmos-sdk/pu...


Near line 30: Possible spelling mistake found.
Context: ...//pull/19537) Changing MinCommissionRate in MsgUpdateParams now updates the m...


Near line 30: Possible spelling mistake found.
Context: .../19537) Changing MinCommissionRate in MsgUpdateParams now updates the minimum commission rat...


Near line 34: Unpaired symbol: ‘[’ seems to be missing
Context: ...validators. ### Improvements * [#19779](https://github.com/cosmos/cosmos-sdk/pu...


Near line 34: Possible spelling mistake found.
Context: ...smos-sdk/pull/19779) Allows for setting unbonding_time to zero. * [#19277](https://gith...


Near line 36: Unpaired symbol: ‘[’ seems to be missing
Context: ...ing unbonding_time to zero. * [#19277](https://github.com/cosmos/cosmos-sdk/pu...


Near line 36: Possible spelling mistake found.
Context: ...s/cosmos-sdk/pull/19277) Hooks calls on SetUnbondingDelegationEntry, SetRedelegationEntry, Slash and `...


Near line 36: Possible spelling mistake found.
Context: ...calls on SetUnbondingDelegationEntry, SetRedelegationEntry, Slash and RemoveValidator returns...


Near line 36: Possible spelling mistake found.
Context: ...y, SetRedelegationEntry, SlashandRemoveValidator` returns errors instead of logging just...


Near line 37: Unpaired symbol: ‘[’ seems to be missing
Context: ...g just like other hooks calls. * [#18636](https://github.com/cosmos/cosmos-sdk/pu...


Near line 37: Possible spelling mistake found.
Context: ...ithub.com//pull/18636) IterateBondedValidatorsByPower, GetDelegatorBonded, Delegate, `Un...


Near line 37: Possible spelling mistake found.
Context: ...8636) IterateBondedValidatorsByPower, GetDelegatorBonded, Delegate, Unbond, Slash, Jail...


Near line 37: Possible spelling mistake found.
Context: ...wer, GetDelegatorBonded, Delegate, Unbond, Slash, Jail, SlashRedelegation`,...


Near line 37: Possible spelling mistake found.
Context: ... Delegate, Unbond, Slash, Jail, SlashRedelegation, ApplyAndReturnValidatorSetUpdates m...


Near line 37: Possible spelling mistake found.
Context: ..., Slash, Jail, SlashRedelegation, ApplyAndReturnValidatorSetUpdates methods no longer panics on any kind o...


Near line 38: Unpaired symbol: ‘[’ seems to be missing
Context: ...ad returns appropriate errors. * [#18506](https://github.com/cosmos/cosmos-sdk/pu...


Near line 38: Possible spelling mistake found.
Context: ...18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic. #...


Near line 38: Possible spelling mistake found.
Context: ...ect the length of the ed25519 pubkey in CreateValidator to prevent panic. ### Bug Fixes * [#1...


Near line 42: Unpaired symbol: ‘[’ seems to be missing
Context: ...prevent panic. ### Bug Fixes * [#19226](https://github.com/cosmos/cosmos-sdk/pu...


Near line 42: Possible spelling mistake found.
Context: ...om//pull/19226) Ensure GetLastValidators in x/staking does not return an erro...


Near line 42: Possible spelling mistake found.
Context: .../stakingdoes not return an error whenMaxValidators` exceeds total number of bonded validat...


Near line 46: Unpaired symbol: ‘[’ seems to be missing
Context: ...rs. ### API Breaking Changes * [#20238](https://github.com/cosmos/cosmos-sdk/pu...


Near line 46: Possible spelling mistake found.
Context: ...ithub.com//pull/20238) NewKeeper now accepts a core/comet.Service as ...


Near line 46: Add a space between sentences.
Context: ...k/pull/20238) NewKeeper now accepts a core/comet.Service as its last argument. * [#19788](http...


Near line 47: Unpaired symbol: ‘[’ seems to be missing
Context: ...ervice` as its last argument. * [#19788](https://github.com/cosmos/cosmos-sdk/pu...


Near line 47: Possible spelling mistake found.
Context: ...om//pull/19788) Remove ABCIValidatorUpdate and ABCIValidatorUpdateZero, use `Mo...


Near line 47: Possible spelling mistake found.
Context: ...19788) Remove ABCIValidatorUpdate and ABCIValidatorUpdateZero, use ModuleValidatorUpdate and `Modu...


Near line 47: Possible spelling mistake found.
Context: ...ateandABCIValidatorUpdateZero, use ModuleValidatorUpdateandModuleValidatorUpdateIsZero` inst...


Near line 47: Possible spelling mistake found.
Context: ...eZero, use ModuleValidatorUpdateandModuleValidatorUpdateIsZero` instead. * [#19754](https://github.com...


Near line 48: Unpaired symbol: ‘[’ seems to be missing
Context: ...alidatorUpdateIsZero` instead. * [#19754](https://github.com/cosmos/cosmos-sdk/pu...


Near line 48: Add a space between sentences.
Context: ...os/cosmos-sdk/pull/19754) Update to use []appmodule.ValidatorUpdate as return for `ApplyAndReturnValidator...


Near line 48: Possible spelling mistake found.
Context: ...ppmodule.ValidatorUpdateas return forApplyAndReturnValidatorSetUpdates`. * [#19414](https://github.com/cosmos/...


Near line 49: Unpaired symbol: ‘[’ seems to be missing
Context: ...AndReturnValidatorSetUpdates`. * [#19414](https://github.com/cosmos/cosmos-sdk/pu...


Near line 49: Possible spelling mistake found.
Context: ...ithub.com//pull/19414) NewStakingKeeper takes an environment variable instead ...


Near line 50: Unpaired symbol: ‘[’ seems to be missing
Context: ...nstead of individual services. * [#19742](https://github.com/cosmos/cosmos-sdk/pu...


Near line 50: Possible spelling mistake found.
Context: ...ithub.com//pull/19742) NewStakeAuthorization now takes address.Codec as argument....


Near line 50: Add a space between sentences.
Context: ...9742) NewStakeAuthorization now takes address.Codec as argument. * [#19735](https://github...


Near line 51: Unpaired symbol: ‘[’ seems to be missing
Context: ...s address.Codec as argument. * [#19735](https://github.com/cosmos/cosmos-sdk/pu...


Near line 51: Possible spelling mistake found.
Context: ...s/cosmos-sdk/pull/19735) Update genesis api to match new appmodule.HasGenesis int...


Near line 51: Add a space between sentences.
Context: .../19735) Update genesis api to match new appmodule.HasGenesis interface. * [#18198](https://github.c...


Near line 52: Unpaired symbol: ‘[’ seems to be missing
Context: ...pmodule.HasGenesis` interface. * [#18198](https://github.com/cosmos/cosmos-sdk/pu...


Near line 52: Possible spelling mistake found.
Context: .../cosmos-sdk/pull/18198) Validator and Delegator interfaces were moved to `github.com/c...


Near line 53: Unpaired symbol: ‘[’ seems to be missing
Context: ...y on staking in other modules. * [#17778](https://github.com/cosmos/cosmos-sdk/pu...


Near line 54: Possible spelling mistake found.
Context: ...or Params * remove from Keeper: GetParams, SetParams * [#17486](https://github...


Near line 55: Unpaired symbol: ‘[’ seems to be missing
Context: ...per: GetParams, SetParams` * [#17486](https://github.com/cosmos/cosmos-sdk/pu...


Near line 55: Possible spelling mistake found.
Context: ...mos-sdk/pull/17486) Use collections for RedelegationQueueKey: * remove from types: `GetRedele...


Near line 58: Unpaired symbol: ‘[’ seems to be missing
Context: ...: RedelegationQueueIterator` * [#17562](https://github.com/cosmos/cosmos-sdk/pu...


Near line 59: Possible spelling mistake found.
Context: ...idatorQueue * remove fromtypes: GetValidatorQueueKey, ParseValidatorQueueKey` * remove...


Near line 61: Unpaired symbol: ‘[’ seems to be missing
Context: ...per: ValidatorQueueIterator` * [#17498](https://github.com/cosmos/cosmos-sdk/pu...


Near line 61: Possible spelling mistake found.
Context: ...mos-sdk/pull/17498) Use collections for LastValidatorPower: * remove from types: `GetLastVa...


Near line 63: Possible spelling mistake found.
Context: ...orPowerKey * remove fromKeeper: LastValidatorsIterator, IterateLastValidators` * [#17291](ht...


Near line 64: Unpaired symbol: ‘[’ seems to be missing
Context: ...ator, IterateLastValidators` * [#17291](https://github.com/cosmos/cosmos-sdk/pu...


Near line 64: Possible spelling mistake found.
Context: ...mos-sdk/pull/17291) Use collections for UnbondingDelegationByValIndex: * remove from types: `GetUBDKey...


Near line 65: Possible spelling mistake found.
Context: ...ByValIndex: * remove from types: GetUBDKeyFromValIndexKey, GetUBDsByValIndexKey, GetUBDByValI...


Near line 65: Possible spelling mistake found.
Context: ...om types: GetUBDKeyFromValIndexKey, GetUBDsByValIndexKey, GetUBDByValIndexKey * (x/slashing) ...


Near line 66: Possible spelling mistake found.
Context: ...mos-sdk/pull/17568) Use collections for ValidatorMissedBlockBitmap: * remove from types: `Validator...


Near line 67: Possible spelling mistake found.
Context: ...lockBitmap: * remove from types: ValidatorMissedBlockBitmapPrefixKey, ValidatorMissedBlockBitmapKey` * [#1...


Near line 68: Unpaired symbol: ‘[’ seems to be missing
Context: ...ValidatorMissedBlockBitmapKey` * [#17481](https://github.com/cosmos/cosmos-sdk/pu...


Near line 68: Possible spelling mistake found.
Context: ...mos-sdk/pull/17481) Use collections for UnbondingQueue: * remove from Keeper: `UBDQueue...


Near line 71: Unpaired symbol: ‘[’ seems to be missing
Context: ...GetUnbondingDelegationTimeKey` * [#17123](https://github.com/cosmos/cosmos-sdk/pu...


Near line 72: Unpaired symbol: ‘[’ seems to be missing
Context: ...e collections for Validators * [#17270](https://github.com/cosmos/cosmos-sdk/pu...


Near line 72: Possible spelling mistake found.
Context: ...mos-sdk/pull/17270) Use collections for UnbondingDelegation: * remove from types: `GetUBDsKe...


Near line 74: Possible spelling mistake found.
Context: ...GetUBDsKey * remove fromKeeper: IterateUnbondingDelegations, IterateDelegatorUnbondingDelegations...


Near line 75: Unpaired symbol: ‘[’ seems to be missing
Context: ...DelegatorUnbondingDelegations` * [#17336](https://github.com/cosmos/cosmos-sdk/pu...


Near line 75: Possible spelling mistake found.
Context: ...mos-sdk/pull/17336) Use collections for RedelegationByValDstIndexKey: * remove from types: `GetREDByV...


Near line 76: Possible spelling mistake found.
Context: ...stIndexKey: * remove from types: GetREDByValDstIndexKey, GetREDsToValDstIndexKey` * [#17332](...


Near line 77: Unpaired symbol: ‘[’ seems to be missing
Context: ...ey, GetREDsToValDstIndexKey` * [#17332](https://github.com/cosmos/cosmos-sdk/pu...


Near line 77: Possible spelling mistake found.
Context: ...mos-sdk/pull/17332) Use collections for RedelegationByValSrcIndexKey: * remove from types: `GetREDKey...


Near line 78: Possible spelling mistake found.
Context: ...rcIndexKey: * remove from types: GetREDKeyFromValSrcIndexKey, GetREDsFromValSrcIndexKey` * [#17315...


Near line 79: Unpaired symbol: ‘[’ seems to be missing
Context: ..., GetREDsFromValSrcIndexKey` * [#17315](https://github.com/cosmos/cosmos-sdk/pu...


Near line 79: Possible spelling mistake found.
Context: ...mos-sdk/pull/17315) Use collections for RedelegationKey: * remove from keeper: `GetRedel...


Near line 81: Unpaired symbol: ‘[’ seems to be missing
Context: ...om keeper: GetRedelegation * [#17260](https://github.com/cosmos/cosmos-sdk/pu...


Near line 81: Possible spelling mistake found.
Context: ...mos-sdk/pull/17260) Use collections for DelegationKey: * remove from types: `GetDelega...


Near line 82: Possible spelling mistake found.
Context: ...egationKey: * remove from types: GetDelegationKey, GetDelegationsKey` * [#17288](https:...


Near line 83: Unpaired symbol: ‘[’ seems to be missing
Context: ...ationKey, GetDelegationsKey` * [#17288](https://github.com/cosmos/cosmos-sdk/pu...


Near line 83: Possible spelling mistake found.
Context: ...mos-sdk/pull/17288) Use collections for UnbondingIndex: * remove from types: `GetUnbond...


Near line 84: Possible spelling mistake found.
Context: ...ndingIndex: * remove from types: GetUnbondingIndexKey`. * [#17256](https://github.com/cosmos/...


Near line 85: Unpaired symbol: ‘[’ seems to be missing
Context: ...ypes: GetUnbondingIndexKey`. * [#17256](https://github.com/cosmos/cosmos-sdk/pu...


Near line 85: Possible spelling mistake found.
Context: ...mos-sdk/pull/17256) Use collections for UnbondingID. * [#17260](https://github.com/cosmos/...


Near line 86: Unpaired symbol: ‘[’ seems to be missing
Context: ...collections for UnbondingID. * [#17260](https://github.com/cosmos/cosmos-sdk/pu...


Near line 86: Possible spelling mistake found.
Context: ...mos-sdk/pull/17260) Use collections for ValidatorByConsAddr: * remove from types: `GetValida...


Near line 88: Unpaired symbol: ‘[’ seems to be missing
Context: ...: GetValidatorByConsAddrKey` * [#17248](https://github.com/cosmos/cosmos-sdk/pu...


Near line 88: Possible spelling mistake found.
Context: ...mos-sdk/pull/17248) Use collections for UnbondingType. * remove from types: `GetUnbond...


Near line 89: This sentence does not start with an uppercase letter.
Context: ... collections for UnbondingType. * remove from types: GetUnbondingTypeKey. * ...


Near line 89: Possible spelling mistake found.
Context: ...ondingType. * remove from types: GetUnbondingTypeKey`. * [#17063](https://github.com/cosmos/...


Near line 90: Unpaired symbol: ‘[’ seems to be missing
Context: ...types: GetUnbondingTypeKey`. * [#17063](https://github.com/cosmos/cosmos-sdk/pu...


Near line 90: Possible spelling mistake found.
Context: ...mos-sdk/pull/17063) Use collections for HistoricalInfo: * remove Keeper: `GetHistorical...


Near line 91: Possible spelling mistake found.
Context: ...HistoricalInfo: * remove Keeper: GetHistoricalInfo, SetHistoricalInfo` * [#17062](https:...


Near line 92: Unpaired symbol: ‘[’ seems to be missing
Context: ...icalInfo, SetHistoricalInfo` * [#17062](https://github.com/cosmos/cosmos-sdk/pu...


Near line 92: Possible spelling mistake found.
Context: ...om//pull/19788) Remove GetValidatorUpdates and ValidatorUpdates storage. * [#17...


Near line 92: Possible spelling mistake found.
Context: ...19788) Remove GetValidatorUpdates and ValidatorUpdates storage. * [#17026](https://github.com...


Near line 93: Unpaired symbol: ‘[’ seems to be missing
Context: ...nd ValidatorUpdates storage. * [#17026](https://github.com/cosmos/cosmos-sdk/pu...


Near line 93: Possible spelling mistake found.
Context: ...mos-sdk/pull/17026) Use collections for LastTotalPower: * remove Keeper: `SetLastTotalP...


Near line 94: Possible spelling mistake found.
Context: ...LastTotalPower: * remove Keeper: SetLastTotalPower, GetLastTotalPower` * [#17335](https:...


Near line 95: Unpaired symbol: ‘[’ seems to be missing
Context: ...talPower, GetLastTotalPower` * [#17335](https://github.com/cosmos/cosmos-sdk/pu...


Near line 95: Add a space between sentences.
Context: .../cosmos-sdk/pull/17335) Remove usage of "cosmossdk.io/x/staking/types".Infraction_* in favour of `"cosmossdk.io/api/cosm...


Near line 95: Possible spelling mistake. ‘favour’ is British English.
Context: ...dk.io/x/staking/types".Infraction_*in favour of"cosmossdk.io/api/cosmos/staking/v1...


Near line 95: Add a space between sentences.
Context: ...aking/types".Infraction_*in favour of"cosmossdk.io/api/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between...


Near line 95: Consider a shorter alternative to avoid wordiness.
Context: ...pi/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on st...


Near line 96: Unpaired symbol: ‘[’ seems to be missing
Context: ...ncy between modules on staking * [#17655](https://github.com/cosmos/cosmos-sdk/pu...


Near line 96: Possible spelling mistake found.
Context: ...ithub.com//pull/17655) QueryHistoricalInfo was adjusted to return `HistoricalReco...


Near line 96: Possible spelling mistake found.
Context: ...yHistoricalInfowas adjusted to returnHistoricalRecordand markedHist` as deprecated. ### ...


Near line 100: Unpaired symbol: ‘[’ seems to be missing
Context: .... ### State Breaking changes * [#18841](https://github.com/cosmos/cosmos-sdk/pu...


Near line 100: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...


Near line 100: Possible spelling mistake found.
Context: ....com//pull/18841) In a undelegation or redelegation if the shares being lef...


Near line 100: Possible spelling mistake found.
Context: ...os-sdk/pull/18841) In a undelegation or redelegation if the shares being left delegated corr...


Near line 100: Possible spelling mistake found.
Context: ...orrespond to less than 1 token (in base denom) the entire delegation gets removed. * ...


Near line 101: Unpaired symbol: ‘[’ seems to be missing
Context: ...ntire delegation gets removed. * [#18142](https://github.com/cosmos/cosmos-sdk/pu...


Near line 101: Please check whether ‘feel’ might be the correct word here instead of ‘fee’.
Context: .../pull/18142) Introduce key_rotation_fee param to calculate fees while rotating...


Near line 101: Possible spelling mistake found.
Context: ...ull/18142) Introduce key_rotation_fee param to calculate fees while rotating the ke...


Near line 102: Unpaired symbol: ‘[’ seems to be missing
Context: ...e fees while rotating the keys * [#17655](https://github.com/cosmos/cosmos-sdk/pu...


Near line 102: Possible spelling mistake found.
Context: ...ithub.com//pull/17655) HistoricalInfo was replaced with HistoricalRecord, ...


Near line 102: Possible spelling mistake found.
Context: ...655) HistoricalInfo was replaced with HistoricalRecord, it removes the validator set and come...


Near line 103: Unpaired symbol: ‘[’ seems to be missing
Context: ...y keep what is needed for IBC. * [#19740](https://github.com/cosmos/cosmos-sdk/pu...


Near line 103: Possible spelling mistake found.
Context: ...ithub.com//pull/19740) InitGenesis and ExportGenesis module code and ke...


Near line 103: Possible spelling mistake found.
Context: ...osmos-sdk/pull/19740) InitGenesis and ExportGenesis module code and keeper code do not pan...

Path-based Instructions (3)
x/slashing/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/evidence/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/staking/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"


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

@kocubinski kocubinski marked this pull request as ready for review May 1, 2024 20:18
@kocubinski kocubinski requested a review from a team as a code owner May 1, 2024 20:18
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.

Actionable comments posted: 1

Out of diff range and nitpick comments (2)
CHANGELOG.md (2)

43-43: Add a blank line before the "Features" heading for proper markdown formatting.


44-44: Add a blank line before and after the list of features for proper markdown formatting.

Comment on lines +15 to +17
func (c ContextAwareCometInfoService) CometInfo(ctx context.Context) corecomet.Info {
return sdk.UnwrapSDKContext(ctx).CometInfo()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The method CometInfo directly unwraps the SDK context to fetch CometInfo. Consider error handling if the context cannot be unwrapped or if CometInfo is not available.

- return sdk.UnwrapSDKContext(ctx).CometInfo()
+ sdkCtx, ok := sdk.UnwrapSDKContext(ctx)
+ if !ok {
+     return nil, fmt.Errorf("invalid context provided")
+ }
+ return sdkCtx.CometInfo(), nil

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (c ContextAwareCometInfoService) CometInfo(ctx context.Context) corecomet.Info {
return sdk.UnwrapSDKContext(ctx).CometInfo()
}
func (c ContextAwareCometInfoService) CometInfo(ctx context.Context) (corecomet.Info, error) {
sdkCtx, ok := sdk.UnwrapSDKContext(ctx)
if !ok {
return nil, fmt.Errorf("invalid context provided")
}
return sdkCtx.CometInfo(), nil
}

Copy link
Contributor

@samricotta samricotta left a comment

Choose a reason for hiding this comment

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

Looks good! Do we have tests for environment?

@tac0turtle
Copy link
Member

cometinfo shouldnt be in environment. This was avoided so we are not tying environment to a consensus protocol

@samricotta
Copy link
Contributor

samricotta commented May 2, 2024

At what level would we have consensus then? In terms of accessing data

- alias core/comet to core/abci
- refactor usages of core/comet throughout SDK
@kocubinski
Copy link
Member Author

cometinfo shouldnt be in environment. This was avoided so we are not tying environment to a consensus protocol

After offline discussion I floated renaming core/cometinfo -> core/abciinfo since this seems more in line with the spirit of what it represents, and in actuality creates no hard dependency on cometbft libraries, it's more the abci layer.

the last commit does that and refactors usages through the sdk to use core/abci instead of core/comet, but keeps the alias for backwards compat.

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.

Actionable comments posted: 1

Out of diff range and nitpick comments (1)
baseapp/abci_utils_test.go (1)

20-20: Consider organizing imports to improve readability.

Imports should be grouped in a conventional order: standard library imports, third-party imports, and then local imports. This helps in maintaining a clean and organized codebase.

Comment on lines 759 to 766
lastCommit := coreabci.CommitInfo{
Round: ec.Round,
Votes: make([]comet.VoteInfo, len(ec.Votes)),
Votes: make([]coreabci.VoteInfo, len(ec.Votes)),
}

for i, vote := range ec.Votes {
lastCommit.Votes[i] = comet.VoteInfo{
Validator: comet.Validator{
lastCommit.Votes[i] = coreabci.VoteInfo{
Validator: coreabci.Validator{
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Several files in the codebase still lack specific tests for the extendedCommitToLastCommit function. While general assertions are present in baseapp/abci_utils_test.go, they do not directly address the functionality of extendedCommitToLastCommit. It is recommended to add more targeted tests to ensure the conversion logic is thoroughly verified.

  • File needing attention:
    • baseapp/abci_utils_test.go: Contains general assertions but lacks direct tests for extendedCommitToLastCommit.
Analysis chain

Ensure conversion logic correctness in extendedCommitToLastCommit.

Consider adding more unit tests to verify the conversion logic in extendedCommitToLastCommit, ensuring all necessary information is retained and correctly set up.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the conversion from `abci.ExtendedCommitInfo` to `comet.Info` in `extendedCommitToLastCommit` function retains all necessary information and correctly sets up the `coreabci.CommitInfo`.

# Find all test files that test the `extendedCommitToLastCommit` function.
rg --files-with-matches --type go $'func Test.*extendedCommitToLastCommit' | while read file; do
  # Test: Search for assertions that verify the correctness of the conversion. Expect: At least one assertion per test case.
  rg 'require\.(Equal|NoError)' "$file"
done

Length of output: 87



Script:

#!/bin/bash
# Description: Search for any test files related to the `extendedCommitToLastCommit` function to verify the presence of tests.

# Search for any Go test files in the repository.
rg --files --type go '.*_test\.go$'

# Search for the presence of the function `extendedCommitToLastCommit` in the codebase to confirm its implementation.
rg --files-with-matches --type go 'func .*extendedCommitToLastCommit'

Length of output: 246



Script:

#!/bin/bash
# Description: Inspect identified test files for relevant test cases and assertions related to the `extendedCommitToLastCommit` function.

# Check for relevant test cases in 'vote_extensions_test.go'
echo "Checking vote_extensions_test.go for relevant test cases:"
rg 'func Test.*extendedCommitToLastCommit' tests/integration/staking/keeper/vote_extensions_test.go

# Check for relevant test cases in 'abci_utils_test.go'
echo "Checking abci_utils_test.go for relevant test cases:"
rg 'func Test.*extendedCommitToLastCommit' baseapp/abci_utils_test.go

# Check for assertions in these test cases
echo "Checking for assertions in identified test cases:"
rg 'require\.(Equal|NoError)' tests/integration/staking/keeper/vote_extensions_test.go
rg 'require\.(Equal|NoError)' baseapp/abci_utils_test.go

Length of output: 740

x/evidence/keeper/abci.go Fixed Show fixed Hide fixed
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.

we just need to remove it from env otherwise good

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.

Actionable comments posted: 1

simapp/app.go Outdated
@@ -281,6 +281,7 @@ func NewSimApp(
interfaceRegistry: interfaceRegistry,
keys: keys,
}
cometService := runtime.ContextAwareCometInfoService{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduce CometInfoService with proper initialization.

The CometInfoService is instantiated but not properly initialized. It's crucial to ensure that the service is correctly set up with necessary dependencies or configurations to function as expected. Consider initializing it with required parameters or services.

x/evidence/module.go Dismissed Show dismissed Hide dismissed
x/slashing/module.go Dismissed Show dismissed Hide dismissed
CHANGELOG.md 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.

need to add changelog entries to the modules then we can merge this

Co-authored-by: Marko <marko@baricevic.me>
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.

Actionable comments posted: 4

Out of diff range and nitpick comments (3)
tests/integration/slashing/abci_test.go (1)

Line range hint 107-122: Ensure consistent context updates within the loop.

Consider refactoring the loop to update the context in a single place to enhance readability and reduce the chance of errors in context management.

for ; height < ((signedBlocksWindow * 2) - minSignedPerWindow + 1); height++ {
-   ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height}).WithCometInfo(comet.Info{
+   ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height})
    LastCommit: comet.CommitInfo{Votes: []comet.VoteInfo{{
        Validator:   abciVal,
        BlockIDFlag: comet.BlockIDFlagAbsent,
    }}},
})
tests/integration/evidence/keeper/infraction_test.go (1)

Line range hint 245-258: Handle potential errors in BeginBlocker.

- assert.NilError(t, f.evidenceKeeper.BeginBlocker(ctx.WithCometInfo(nci), cometInfoService))
+ err = f.evidenceKeeper.BeginBlocker(ctx.WithCometInfo(nci), cometInfoService)
+ assert.NilError(t, err)

This change ensures that any errors during the execution of BeginBlocker are caught and handled appropriately, maintaining the robustness of the test.

tests/integration/slashing/keeper/keeper_test.go (1)

Line range hint 1-200: Review of the entire test file shows that existing functionalities are maintained and the integration of cometInfoService does not introduce any regressions.

It's recommended to add more comprehensive tests around the new cometInfoService to ensure all edge cases are covered.

Would you like assistance in drafting additional test cases for cometInfoService?

Comment on lines +87 to +89
cometInfoService := runtime.NewContextAwareCometInfoService()

err = slashing.BeginBlocker(ctx, slashingKeeper)
err = slashing.BeginBlocker(ctx, slashingKeeper, cometInfoService)
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduce error handling for NewContextAwareCometInfoService.

- cometInfoService := runtime.NewContextAwareCometInfoService()
+ cometInfoService, err := runtime.NewContextAwareCometInfoService()
+ require.NoError(t, err)

This change ensures that any errors during the initialization of cometInfoService are caught and handled appropriately, maintaining the robustness of the test.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cometInfoService := runtime.NewContextAwareCometInfoService()
err = slashing.BeginBlocker(ctx, slashingKeeper)
err = slashing.BeginBlocker(ctx, slashingKeeper, cometInfoService)
cometInfoService, err := runtime.NewContextAwareCometInfoService()
require.NoError(t, err)
err = slashing.BeginBlocker(ctx, slashingKeeper, cometInfoService)

@@ -100,7 +100,7 @@ func initFixture(tb testing.TB) *fixture {
authority.String(),
)

stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr))
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling for NewKeeper.

- stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ require.NoError(t, err)

This change ensures that any errors during the initialization of stakingKeeper are caught and handled appropriately, maintaining the robustness of the test.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
require.NoError(t, err)

@@ -155,7 +159,7 @@ func initFixture(tb testing.TB) *fixture {
authority.String(),
)

stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr))
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling for NewKeeper.

- stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ require.NoError(t, err)

This change ensures that any errors during the initialization of stakingKeeper are caught and handled appropriately, maintaining the robustness of the test.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[types.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(queryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
require.NoError(t, err)

@@ -136,7 +137,7 @@ func initFixture(tb testing.TB) *fixture {
authority.String(),
)

stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr))
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling for NewKeeper.

- stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
+ require.NoError(t, err)

This change ensures that any errors during the initialization of stakingKeeper are caught and handled appropriately, maintaining the robustness of the test.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
stakingKeeper, err := stakingkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithRouterService(grpcQueryRouter, msgRouter)), accountKeeper, bankKeeper, authority.String(), addresscodec.NewBech32Codec(sdk.Bech32PrefixValAddr), addresscodec.NewBech32Codec(sdk.Bech32PrefixConsAddr), runtime.NewContextAwareCometInfoService())
require.NoError(t, err)

@kocubinski kocubinski enabled auto-merge May 3, 2024 01:07
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.

Actionable comments posted: 7

Out of diff range and nitpick comments (1)
x/slashing/CHANGELOG.md (1)

Line range hint 44-44: Adjust indentation for list items to match the standard markdown list style.

-    * remove `Keeper`: `SetValidatorSigningInfo`, `GetValidatorSigningInfo`, `IterateValidatorSigningInfos`
+  * remove `Keeper`: `SetValidatorSigningInfo`, `GetValidatorSigningInfo`, `IterateValidatorSigningInfos`

@@ -35,6 +35,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistency in service naming.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument.  `BeginBlocker` now takes in a `core/comet.Service`.
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument.  `BeginBlocker` now takes in a `core/comet.Service`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument. `BeginBlocker` now takes in a `core/comet.Service`.

Correct the grammatical error in the sentence.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument.  `BeginBlocker` now takes in a `core/comet.Service`.
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument.  `BeginBlocker` now takes in a `core/comet.Service`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument. `BeginBlocker` now takes in a `core/comet.Service`.

@@ -35,6 +35,7 @@

### API Breaking Changes

* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
* [#20026](https://github.com/cosmos/cosmos-sdk/pull/20026) Removal of the Address.String() method and related changes:
* `Migrate` now takes a `ValidatorAddressCodec` as argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation for list items to maintain consistency.

-    * `Migrate` now takes a `ValidatorAddressCodec` as argument.
-    * `Migrator` has a new field of `ValidatorAddressCodec` type.
+  * `Migrate` now takes a `ValidatorAddressCodec` as argument.
+  * `Migrator` has a new field of `ValidatorAddressCodec` type.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* `Migrate` now takes a `ValidatorAddressCodec` as argument.
* `Migrate` now takes a `ValidatorAddressCodec` as argument.
* `Migrator` has a new field of `ValidatorAddressCodec` type.

@@ -27,6 +27,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Api Breaking Changes

* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistency in service naming and correct grammatical errors.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument.  `BeginBlocker` now takes in a `core/comet.Service`.
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument.  `BeginBlocker` now takes in a `core/comet.Service`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument. `BeginBlocker` now takes in a `core/comet.Service`.

Add a space between sentences to improve readability.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument.  `BeginBlocker` now takes in a `core/comet.Service`.
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument. `BeginBlocker` now takes in a `core/comet.Service`.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` an argument. `BeginBlocker` now takes in a `core/comet.Service`.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewAppModule` now takes in a `core/comet.Service` as an argument. `BeginBlocker` now takes in a `core/comet.Service`.

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistency in service naming and correct grammatical errors.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument. 
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.

Add a space between sentences to improve readability.

- * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument. 
+ * [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.
* [#20238](https://github.com/cosmos/cosmos-sdk/pull/20238) `NewKeeper` now accepts a `core/comet.Service` as its last argument.

@kocubinski kocubinski added this pull request to the merge queue May 3, 2024
Merged via the queue into main with commit a6bc921 May 3, 2024
65 of 66 checks passed
@kocubinski kocubinski deleted the kocu/19599-v2 branch May 3, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: add consensus message to slashing & evidence
3 participants