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

chore: rename proofXyz -> xyzProof #5599

Merged

Conversation

crodriguezvega
Copy link
Contributor

Description

I will rely on @DimitrisJim's grep superpowers to pinpoint which ones I am still missing!

closes: #4053
closes: #3726

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.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2024

Codecov Report

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

Comparison is base (dd5b172) 81.18% compared to head (028d5a6) 81.22%.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5599      +/-   ##
==========================================
+ Coverage   81.18%   81.22%   +0.04%     
==========================================
  Files         197      197              
  Lines       15247    15258      +11     
==========================================
+ Hits        12378    12394      +16     
+ Misses       2404     2399       -5     
  Partials      465      465              
Files Coverage Δ
modules/core/02-client/keeper/client.go 94.30% <100.00%> (ø)
modules/core/02-client/types/keys.go 75.00% <ø> (ø)
modules/core/02-client/types/msgs.go 85.22% <100.00%> (ø)
modules/core/02-client/types/params.go 100.00% <100.00%> (ø)
modules/core/03-connection/keeper/handshake.go 88.58% <100.00%> (ø)
modules/core/03-connection/keeper/verify.go 78.79% <100.00%> (ø)
modules/core/03-connection/types/msgs.go 96.02% <100.00%> (ø)
modules/core/04-channel/keeper/handshake.go 89.64% <100.00%> (ø)
modules/core/04-channel/keeper/timeout.go 95.39% <100.00%> (ø)
modules/core/04-channel/keeper/upgrade.go 92.22% <100.00%> (ø)
... and 3 more

... and 2 files with indirect coverage changes

@DimitrisJim
Copy link
Contributor

Seeing a couple of spots (only checked for function args in grep output, testing code and protobuf pb's excluded):

  • module/core/exported/client.go's VerifyUpgradeAndUpdateState has a two more occurences proofUpgradeClient and proofUpgradeConsState (similarly so in the impl's of the interface in wasm and tendermint)
  • modules/core/02-client/keeper/client.go's UpdateClient has proofUpgradeClient and proofUpgradeConsState; function to create a new msg for it also contains same names.
  • modules/core/04-channel/types/msgs.go's NewMsgTimeoutOnClose has proofUnreceived
  • modules/core/04-channel/types/expected_keepers.go's VerifyChannelUpgradeError has proofErrorReceipt (though in the rest of the interface we use a plain proof 🤷)

saw a bunch of em in testing code, maybe that can be left for other PR since they do appear to be plenty of em (also in pb gen'ed code but that would be API breaking).

@crodriguezvega
Copy link
Contributor Author

Yay, thanks, @DimitrisJim!

  • modules/core/04-channel/types/expected_keepers.go's VerifyChannelUpgradeError has proofErrorReceipt (though in the rest of the interface we use a plain proof 🤷)

Yeah, I noticed that as well. I could change those names to be more specific as well, if people prefer.

saw a bunch of em in testing code, maybe that can be left for other PR since they do appear to be plenty of em (also in pb gen'ed code but that would be API breaking).

Indeed, I left all the proto and pb.go files untouched.

@DimitrisJim
Copy link
Contributor

yea, seeing connection's verify funcs generally follow a different pattern (proof and height instead of xyzProof and proofHeigh). Sound fine to clean up here but would also be willing to open additional issue and leave it to external contributors.

@damiannolan
Copy link
Member

modules/core/04-channel/types/expected_keepers.go's VerifyChannelUpgradeError has proofErrorReceipt (though in the rest of the interface we use a plain proof

Agree, would be nice if they all just used proof in there 👍🏻

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.

LGTM, can do a rename in tests afterwards if people want.

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.

My vote would be for dropping the counterparty prefix for proof vars. Otherwise LGTM! Thank you @crodriguezvega and @DimitrisJim 🙏🏻

modules/core/04-channel/keeper/timeout.go Show resolved Hide resolved
modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved
@crodriguezvega crodriguezvega merged commit 9184de3 into main Jan 16, 2024
77 of 78 checks passed
@crodriguezvega crodriguezvega deleted the carlos/4053-use-consistent-naming-throughout-handlers branch January 16, 2024 13:28
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* chore: rename proofXyz -> xyzProof

* more renaming

* rename VerifyChannelUpgradeError proof parameters for consistency with the other verification functions

* remove counterparty prefix from proof names

---------

Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
(cherry picked from commit 9184de3)

# Conflicts:
#	docs/docs/03-light-clients/01-developer-guide/05-upgrades.md
#	modules/core/02-client/keeper/client_test.go
#	modules/core/04-channel/types/msgs.go
#	modules/core/ante/ante_test.go
#	modules/core/keeper/msg_server_test.go
#	modules/light-clients/08-wasm/types/upgrade.go
#	modules/light-clients/08-wasm/types/upgrade_test.go
crodriguezvega pushed a commit that referenced this pull request Jan 16, 2024
* chore: rename `proofXyz` -> `xyzProof` (#5599)

* chore: rename proofXyz -> xyzProof

* more renaming

* rename VerifyChannelUpgradeError proof parameters for consistency with the other verification functions

* remove counterparty prefix from proof names

---------

Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
(cherry picked from commit 9184de3)

# Conflicts:
#	docs/docs/03-light-clients/01-developer-guide/05-upgrades.md
#	modules/core/02-client/keeper/client_test.go
#	modules/core/04-channel/types/msgs.go
#	modules/core/ante/ante_test.go
#	modules/core/keeper/msg_server_test.go
#	modules/light-clients/08-wasm/types/upgrade.go
#	modules/light-clients/08-wasm/types/upgrade_test.go

* resolve conflicts

* delete docs

---------

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
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.

Use consistent naming throughout handlers rename proof params to differentiate from proofHeight param
4 participants