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.

Sign up for GitHub

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

Jump to bottom

Consumer panic cleanup #655

Merged
merged 10 commits into from
Jan 10, 2023
Merged

Consumer panic cleanup #655

merged 10 commits into from
Jan 10, 2023

Conversation

mpoke
Copy link
Contributor

@mpoke mpoke commented Jan 10, 2023

Description

Audit of the panic calls in x/consumer

Linked issues

Works toward #621

Type of change

  • Refactor: Changes existing code style, naming, structure, etc.
  • Docs: Adds documentation

Regression tests

The existing UTs are covering the methods modified.

@mpoke mpoke requested a review from MSalopek January 10, 2023 13:52
mpoke
mpoke commented Jan 10, 2023
View reviewed changes
x/ccv/consumer/keeper/relay.go
@@ -99,9 +96,6 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
// the consumer chain.
func (k Keeper) QueueVSCMaturedPackets(ctx sdk.Context) {
for _, maturityTime := range k.GetElapsedPacketMaturityTimes(ctx) {
if ctx.BlockTime().Before(maturityTime.MaturityTime) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Redundant check.

mpoke
mpoke commented Jan 10, 2023
View reviewed changes
x/ccv/utils/utils.go
@@ -44,15 +44,6 @@ func AccumulateChanges(currentChanges, newChanges []abci.ValidatorUpdate) []abci
return out
}

func GetChangePubKeyAddress(change abci.ValidatorUpdate) (addr []byte) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as it was called only from ApplyCCValidatorChanges. Added the logic directly there.

mpoke
mpoke commented Jan 10, 2023
View reviewed changes
x/ccv/consumer/keeper/validators.go
Comment on lines +19 to +26
// convert TM pubkey to SDK pubkey
pubkey, err := cryptocodec.FromTmProtoPublicKey(change.GetPubKey())
if err != nil {
// An error here would indicate that the validator updates
// received from the provider are invalid.
panic(err)
}
addr := pubkey.Address()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here the logic from utils.GetChangePubKeyAddress.

MSalopek
MSalopek approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@MSalopek MSalopek left a comment

Choose a reason for hiding this comment

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

LGTM!

I researched some known reasons for proto.Marshal() failures.

Usually it will only happen if you try to marshal a nil type or if a string type contains invalid UTF-8 characters.

danwt
danwt approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

Looks fine 👍

@mpoke mpoke mentioned this pull request Jan 10, 2023
Closed
4 tasks
shaspitz
shaspitz approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

Nice!

@mpoke mpoke merged commit 2cbbd10 into main Jan 10, 2023
@mpoke mpoke deleted the marius/panic-consumer branch January 10, 2023 16:48
ThanhNhann pushed a commit to decentrio/interchain-security that referenced this pull request Jan 3, 2024
@ramacarlucho @colin-axner
@seantking @dependabot @damiannolan @crodriguezvega @tac0turtle @fedekunze
fix: support custom chain IDs for testing (cosmos#774)
8dfbc9c 
* Backport commits from main to v3 release branch (cosmos#682)

* reorganize channel handshake handler (cosmos#647)

* reorganize channel handshake handler

split out channel state changes into its own function.

* readjust 27-interchain-accounts to not rely on state being set before the application callback

* add changelog and migration doc entry

* Update modules/core/04-channel/keeper/handshake.go

* docs: ICA Overview (cosmos#626)

* docs: ica overview

* fix: ordering

* add spacing

* fix: spacing

* fix: remove bulletpoints

* fix: wording

* update go mod for security vulnerabilities (cosmos#655)

* update go mod for security vulnerabilities

* update package-lock.json

* update vue dependency (cosmos#662)

* bump glob-parent version in json package (cosmos#663)

* build(deps): bump actions/setup-go from 2.1.4 to 2.1.5 (cosmos#656)

Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2.1.4 to 2.1.5.
- [Release notes](https://github.com/actions/setup-go/releases)
- [Commits](actions/setup-go@v2.1.4...v2.1.5)

---
updated-dependencies:
- dependency-name: actions/setup-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: colin axnér <[email protected]>

* docs: begin removal of internal "spec" directories (cosmos#634)

* begin removal of spec docs within core ibc

* remove broken link

* Apply suggestions from code review

Co-authored-by: Damian Nolan <[email protected]>

* remove broken link

* remove broken links

* Apply suggestions from code review

Co-authored-by: Carlos Rodriguez <[email protected]>

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

* Modify `OnChanOpenTry` application callback to perform app version negotitation (cosmos#646)

* remove NegotiateAppVersion and AppVersion gRPC (cosmos#643)

The NegotiateAppVersion callback has been removed from the IBC Application interface.
The gRPC AppVersion has been removed.
The app version negoitation will be handled by applications by returning the version in OnChanOpenTry.

* Modify `OnChanOpenTry` to return application version (cosmos#650)

* modify OnChanOpenTry to return negotiated version

modify IBCModule interface function OnChanOpenTry to return the negotiated app version. Tests have not been updated

* fix ibc_module_test.go tests

* fix tests

* Apply suggestions from code review

* add handshake test case

* add CHANGELOG and migration docs

* update documentation

* fix broken link

* fix broken link (cosmos#664)

* chore: update make build-docs, add docs build checker (cosmos#667)

* update Makefile, add docs build checker

* Update .github/workflows/check-docs.yml

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

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

* register ICA query server, fix panics in params query cli (cosmos#666)

Register the controller and host query servers to a chain.
Returns an error upon cli params query failure instead of panicing.

* update of roadmap with latest release (cosmos#653)

* update of roadmap with latest release and changed the way release versions are encoded

* fixed typo

Co-authored-by: Carlos Rodriguez <[email protected]>

* build(deps): bump actions/checkout from 2.3.1 to 2.4.0 (cosmos#672)

Bumps [actions/checkout](https://github.com/actions/checkout) from 2.3.1 to 2.4.0.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2.3.1...v2.4.0)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

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

* allow ics20 to connect to middleware (cosmos#675)

* allow ics20 to connect to middleware

Creates ics4Wrapper which allows middleware applications to only implement SendPacket to connect ics20 as an application in its middleware stack

* add changelog and migration doc

* fix migration doc spelling

* fix: register InterchainAccount as x/auth GenesisAccount (cosmos#676)

* adding GenesisAccount interface registration for InterchainAccount impl

* updating RegisterInterfaces ica godoc

* enable mergify for backports (cosmos#678)

Co-authored-by: Carlos Rodriguez <[email protected]>

Co-authored-by: Sean King <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

* fixes evmos tests

* cleanup

* changelog and cleanup

* pr suggestions. bump chainid

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Sean King <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaspitz shaspitz shaspitz approved these changes

@MSalopek MSalopek MSalopek approved these changes

@danwt danwt danwt approved these changes

@jtremback jtremback Awaiting requested review from jtremback

@sainoe sainoe Awaiting requested review from sainoe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mpoke @danwt @MSalopek @shaspitz