refactor: rename SelfClientValidator to ConsensusHost and mv tests #6018
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
I kinda feel like this should be in 07-tendermint. There should be no cycles as 02-client keeper pkg already imports ibctm.
The new keeper func default instantation would just be changed to:
return Keeper{
storeKey: key,
cdc: cdc,
router: router,
- consensusHost: NewTendermintConsensusHost(sk),
+ consensusHost: ibctm.NewConsensusHost(sk),
legacySubspace: legacySubspace,
stakingKeeper: sk,
upgradeKeeper: uk,
}We could later remove the default instantiation and force a types.ConsensusHost impl as an arg to NewKeeper in a major release (panic if nil, similarly to other expected interfaces on node startup).
There was a problem hiding this comment.
Makes sense to move it to 07-tendermint. 👍
There was a problem hiding this comment.
Great, I'll move it there then
damiannolan
requested review from
colin-axner,
AdityaSripal,
charleenfei,
chatton,
DimitrisJim,
srdtrk and
crodriguezvega
as code owners
There was a problem hiding this comment.
Looks good, Damian. I opened a follow-up to do some renaming, let me know what you think.
There was a problem hiding this comment.
Makes sense to move it to 07-tendermint. 👍
There was a problem hiding this comment.
thanks @damiannolan for doing this prep work to merge the feature branch in!
| // historical info is set on BeginBlock in x/staking, which is now encapsulated within the FinalizeBlock abci method, | ||
| // thus, we do not have historical info for current height due to how the ibctesting library operates. | ||
| // ibctesting calls app.Commit() as a final step on NextBlock and we invoke test code before FinalizeBlock is called at the current height once again. |
There was a problem hiding this comment.
Found some unexpected behaviour here with how testing lib operates.
Invoking TrackHistoricalInfo directly resolves this for the time being.
There was a problem hiding this comment.
pretty sweet find, thanks for digging through it!
|
Description
Renames
SelfClientValidatorinterface toConsensusHostand moves associates tests to new test file.part of: #5315
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.