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 warnings when provider unbonding is shorter than consumer unbonding #858

Merged

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Apr 18, 2023

Description

Changes the client code for creating consumer addition proposals so that a warning is printed when
the unbonding period in the proposal is >= than the unbonding period of the provider.

To test anything here, I needed to create a mock of the CometBFT RPC client. I didn't find anything like this elsewhere, but lmk if it already exists.

For the error message, I don't think the logger in the app is in scope here, so I just print to stderr.

Linked issues

Closes: #733

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

  • Feature: Changes and/or adds code behavior, irrelevant to bug fixes
  • Fix: Changes and/or adds code behavior, specifically to fix a bug
  • Refactor: Changes existing code style, naming, structure, etc.
  • Testing: Adds testing
  • Docs: Adds documentation

New behavior tests

Added a test for the logic that checks unbonding periods of provider and consumer against each other.

Versioning Implications

If the above box is checked, which version should be bumped?

  • MAJOR: Consensus breaking changes to both the provider and consumers(s), including updates/breaking changes to IBC communication between provider and consumer(s)
  • MINOR: Consensus breaking changes which affect either only the provider or only the consumer(s)
  • PATCH: Non consensus breaking changes

Targeting

Please select one of the following:

  • This PR is only relevant to main
  • This PR is relevant to main, and should also be back-ported to ____ (ex: v1.0.0 and v1.1.0)
  • This PR is only relevant to ____ (ex: v1.0.0, v1.1.0, and v1.2.0)

@p-offtermatt p-offtermatt linked an issue Apr 18, 2023 that may be closed by this pull request
2 tasks
@p-offtermatt
Copy link
Contributor Author

p-offtermatt commented Apr 20, 2023

The quality gate is a bit unhelpful here: The new code is in an area where we have 0% test coverage at the moment, since it is concerned with the CLI, for which there are no unit tests. I added tests for my new functionality as much as possible. I'm not sure how to cover more code here, but am open to suggestions. Alternatively, we can ignore the code coverage for this one.

testutil/mock_rpc_client.go Outdated Show resolved Hide resolved
testutil/mock_rpc_client.go Outdated Show resolved Hide resolved
x/ccv/provider/client/proposal_handler.go Outdated Show resolved Hide resolved
x/ccv/provider/client/proposal_handler.go Outdated Show resolved Hide resolved
x/ccv/provider/client/proposal_handler.go Outdated Show resolved Hide resolved
@p-offtermatt
Copy link
Contributor Author

p-offtermatt commented Apr 21, 2023

Summary of changes since the reviews:

  • I get the proper unbonding time from the staking module now, rather than the max evidence age.
  • I removed the test for the functionality, since I needed to call a different client to get the proper unbonding time, so my mock was also obsolete. My reasoning for the removal is that we do not test for CLI interactions at the moment at all. I think doing CLI unit testing properly is out of the scope of this PR, and is maybe something we do not need to do at all, since we have e2e tests that call the cli. My test before was too focused on my implementation, rather than the expected behavior, as seen by the fact that changing the implementation slightly would need a complete rewrite of the test.

@MSalopek MSalopek self-requested a review April 21, 2023 18:24
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.

New changes look good.

Approve!

x/ccv/provider/client/proposal_handler.go Outdated Show resolved Hide resolved
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.

One comment but LGTM!

x/ccv/provider/client/proposal_handler.go Show resolved Hide resolved
@p-offtermatt p-offtermatt merged commit cf366e7 into main Apr 28, 2023
@p-offtermatt p-offtermatt deleted the ph/733-unbonding-period-on-consumers-is-not-enforced branch April 28, 2023 08:38
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.

Unbonding period on consumers is not enforced
4 participants