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

Add revision number tests for 07-tendermint #1302

Merged
merged 18 commits into from
May 10, 2022

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Apr 26, 2022

Description

closes: #1184
closes: #1331


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

adds UpgradeChain which manually upgrades the chainID to the next revision
adds test cases for VerifyClientMessage in relation to a changing revision number
Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

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

Wunderschön!

@colin-axner colin-axner marked this pull request as draft April 28, 2022 09:35
@colin-axner
Copy link
Contributor Author

Going to adjust the handling of revision misbehaviour. Revert to old assumption:

  • if the chainID is revision format, past/previous must be revision formats

In light of #1308 and #1309, this will be safe since arbitrary changing of the chainID will not be supported. If a chain wants to go from a non-revision number chainID to a revision number chainID, they must start at -0. Thus the existing if statement will work correctly. Going to adjust the tests to account for this assumption. Will update the in code documentation as well

@colin-axner
Copy link
Contributor Author

see comment

  • reverted change to if statement
  • updated tests/testing package to use chains with a revision ID format

@colin-axner colin-axner marked this pull request as ready for review April 28, 2022 15:39
@colin-axner
Copy link
Contributor Author

Addresses #1331

Needed to fix a lot of tests to handle the new chain ID using a revision number of 0. Removed some of the hard coding and left other parts

true,
},
{
"valid misbehaviour with trusted heights at a previous revision", func() {
Copy link
Member

Choose a reason for hiding this comment

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

nice work on these tests!


// update counterparty client manually
clientState.ChainId = newChainID
clientState.LatestHeight = clienttypes.NewHeight(revisionNumber+1, clientState.LatestHeight.GetRevisionHeight()+1)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be resetting revision height here to prove that resetting height still works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too difficult right now. Would require resetting the SimApp such that it uses a zero height rather than incrementing its existing height

@colin-axner colin-axner requested a review from AdityaSripal May 2, 2022 12:24
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Nice work! Just a nit on the test comment

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved
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.

Awesome, lgtm!

// and upgrading the client via MsgUpgradeClient
// see reference https://github.com/cosmos/ibc-go/pull/1169
func (endpoint *Endpoint) UpgradeChain() error {
if endpoint.Counterparty.ClientID == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we use strings.TrimSpace() here?

Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

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

amazing work

@colin-axner colin-axner merged commit 31b6ead into 02-client-refactor May 10, 2022
@colin-axner colin-axner deleted the colin/1184-revisionid-tests branch May 10, 2022 10:40
oshorefueled pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022
* add revision tests for VerifyClientMessage

adds UpgradeChain which manually upgrades the chainID to the next revision
adds test cases for VerifyClientMessage in relation to a changing revision number

* fix revision tests

* update revision tests, use revision format as default in testing

* remove hard coding from upgrade tests

* fix misbehaviour handle tests

* add changelog entry

* fix client state tests

* disable updates to previous revisions

* fix hard coded tests

* add test case for unsuccessful update to previous revision

* Update modules/light-clients/07-tendermint/types/update_test.go

Co-authored-by: Aditya <adityasripal@gmail.com>

* add strings trim space

* add tests for non revision chainIDs

Co-authored-by: Aditya <adityasripal@gmail.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