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 connection interface #5770

Merged
merged 5 commits into from
Jan 30, 2024
Merged

Conversation

colin-axner
Copy link
Contributor

Description

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)


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.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@colin-axner colin-axner marked this pull request as ready for review January 30, 2024 12:55
@colin-axner colin-axner added the type: code hygiene Clean up code but without changing functionality or interfaces label Jan 30, 2024
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Beautiful! Love the improvements :)

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@2d210e2). Click here to learn what that means.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5770   +/-   ##
=======================================
  Coverage        ?   81.55%           
=======================================
  Files           ?      199           
  Lines           ?    15180           
  Branches        ?        0           
=======================================
  Hits            ?    12380           
  Misses          ?     2334           
  Partials        ?      466           
Files Coverage Δ
...odules/core/02-client/migrations/v7/solomachine.go 19.40% <ø> (ø)
modules/core/03-connection/keeper/verify.go 80.36% <100.00%> (ø)
modules/core/03-connection/types/codec.go 100.00% <ø> (ø)
modules/core/03-connection/types/connection.go 79.10% <ø> (ø)
modules/core/04-channel/keeper/keeper.go 90.90% <0.00%> (ø)

@@ -158,14 +158,6 @@ func (ClientState) VerifyClientConsensusState(
panic(errors.New("legacy solo machine is deprecated"))
}

// VerifyConnectionState panics!
func (ClientState) VerifyConnectionState(
Copy link
Contributor

@DimitrisJim DimitrisJim Jan 30, 2024

Choose a reason for hiding this comment

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

seems legacy solomachine has quite the legacy methods defined on it (Verify* family of funcs, CheckHeaderAndUpdateState etc)? Do we have an issue to clean those up? Do we have a bigger issue for legacy solomachine might be better q tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the answer is no to both questions! Definitely would be great to have an issue for both. I've just been removing unnecessary funcs as I bump into them

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

🧹

@colin-axner colin-axner merged commit ffd91be into main Jan 30, 2024
76 of 77 checks passed
@colin-axner colin-axner deleted the colin/rm-connection-interface branch January 30, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code hygiene Clean up code but without changing functionality or interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants