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: solomachine remove unused data types #1716

Conversation

damiannolan
Copy link
Member

Description

  • Removes unused data types

NOTE: branch is targeted for #1715 and will cascaded to #1687

closes: #XXXX


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 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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • 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 Report

Merging #1716 (e0d47f9) into damian/solomachine-misbehaviour-refactor (ec8e4c4) will increase coverage by 0.24%.
The diff coverage is 83.33%.

Impacted file tree graph

@@                             Coverage Diff                              @@
##           damian/solomachine-misbehaviour-refactor    #1716      +/-   ##
============================================================================
+ Coverage                                     79.85%   80.10%   +0.24%     
============================================================================
  Files                                           166      166              
  Lines                                         11821    11334     -487     
============================================================================
- Hits                                           9440     9079     -361     
+ Misses                                         1931     1852      -79     
+ Partials                                        450      403      -47     
Impacted Files Coverage Δ
...dules/light-clients/06-solomachine/client_state.go 58.06% <ø> (-10.63%) ⬇️
modules/light-clients/06-solomachine/codec.go 100.00% <ø> (+12.50%) ⬆️
...dules/light-clients/06-solomachine/misbehaviour.go 90.00% <ø> (+23.33%) ⬆️
modules/light-clients/06-solomachine/proof.go 100.00% <ø> (+24.44%) ⬆️
...odules/light-clients/06-solomachine/solomachine.go 100.00% <ø> (ø)
modules/light-clients/06-solomachine/update.go 93.40% <83.33%> (-2.54%) ⬇️

Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

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

🎉 deleting code 🎉

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

🚀

Great work! Thanks for breaking this up, very easy to review. Just left a comment unrelated to this pr (wondering if we should do the change with this series of changes)

}

signBytes := &SignBytesV2{
Sequence: header.Sequence,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given:

// assert update sequence is current sequence
	if header.Sequence != cs.Sequence {
		return sdkerrors.Wrapf(
			clienttypes.ErrInvalidHeader,
			"header sequence does not match the client state sequence (%d != %d)", header.Sequence, cs.Sequence,
		)
}

I wonder if we should just remove the Sequence argument from the header proto type and just use the current client state sequence

Copy link
Member Author

@damiannolan damiannolan Jul 19, 2022

Choose a reason for hiding this comment

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

I'd be happy to do that in this series of PRs if you think its safe to omit in the Header proto. We can open an extra issue and add it to the current workload 👍

@damiannolan damiannolan merged commit 5a59d03 into damian/solomachine-misbehaviour-refactor Jul 19, 2022
@damiannolan damiannolan deleted the damian/solomachine-remove-types branch July 19, 2022 12:49
damiannolan added a commit that referenced this pull request Jul 19, 2022
* adding SignatureAndDataV2 proto message type

* updating misbehaviour checking

* removing dead solomachine code (#1716)
colin-axner added a commit that referenced this pull request Aug 2, 2022
…lification (#1687)

* adding new SignBytes type, generic membership verification implementation and tests

* adding protodocs

* updating comment

* refactor: solomachine misbehaviour checking (#1715)

* adding SignatureAndDataV2 proto message type

* updating misbehaviour checking

* removing dead solomachine code (#1716)

* updating tests with concrete ibc core types

* refactor: solomachine generic VerifyNonMembership (#1720)

* adding verification of non-membership with tests

* refactor common code to produceVerificationArgs

* removing unused produce args func

* Update modules/light-clients/06-solomachine/client_state_test.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* removing V2 suffix from SignBytes and SignatureAndData types

* use current diversifier when verifying header details

* Add test for new diversifier for solomachine (#1860)

* add test for successful new diversifier

* add changelog entry

* fix tests

* restoring solomachine/v2 protos, updadting v2 codegen path and adding solomachine/v3 protobuf defs

* adding changelog entries

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
oshorefueled pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022
…lification (cosmos#1687)

* adding new SignBytes type, generic membership verification implementation and tests

* adding protodocs

* updating comment

* refactor: solomachine misbehaviour checking (cosmos#1715)

* adding SignatureAndDataV2 proto message type

* updating misbehaviour checking

* removing dead solomachine code (cosmos#1716)

* updating tests with concrete ibc core types

* refactor: solomachine generic VerifyNonMembership (cosmos#1720)

* adding verification of non-membership with tests

* refactor common code to produceVerificationArgs

* removing unused produce args func

* Update modules/light-clients/06-solomachine/client_state_test.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* removing V2 suffix from SignBytes and SignatureAndData types

* use current diversifier when verifying header details

* Add test for new diversifier for solomachine (cosmos#1860)

* add test for successful new diversifier

* add changelog entry

* fix tests

* restoring solomachine/v2 protos, updadting v2 codegen path and adding solomachine/v3 protobuf defs

* adding changelog entries

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
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.

4 participants