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.

Sign up for GitHub

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

Jump to bottom

chore: move server.GenerateCoinKey and server.GenerateSaveCoinKey to … #10957

Merged
merged 6 commits into from
Jan 18, 2022
Merged

chore: move server.GenerateCoinKey and server.GenerateSaveCoinKey to … #10957

merged 6 commits into from
Jan 18, 2022

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Jan 17, 2022 •
edited
Loading

Cleaning it and moving the breaking "test" code to testutils
Related to: #10956

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@robert-zaremba robert-zaremba added T: Tests C:Keys Keybase, KMS and HSMs labels Jan 17, 2022
@github-actions github-actions bot removed the C:Keys Keybase, KMS and HSMs label Jan 17, 2022
anilcse
anilcse approved these changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

makes sense as we are not using it anywhere else

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. and removed A:automerge Automatically merge PR once all prerequisites pass. labels Jan 17, 2022
@amaury1093
Copy link
Contributor

Definitely makes sense, but we need to solve the import cycle

@robert-zaremba
Copy link
Collaborator Author

ledger_mock.go cause import cycle. That's bad... testutil is a good place for this functions, because we have testutil.TestMnemonic there as well.

testutil/known_values.go imho should be in testutil/testdata package. What do you think about doing that breaking change?

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Jan 17, 2022 •
edited
Loading

However ... not sure if we can backport it then to 0.45 RC... I would say yes - because it's moving a test object.

@amaury1093
Copy link
Contributor

ledger_mock.go cause import cycle. That's bad... testutil is a good place for this functions, because we have testutil.TestMnemonic there as well.

Makes sense to me 👍

For v0.45, I would really prefer to not break any API. can we just do some patch like #10935 (comment) ?

@amaury1093 amaury1093 self-assigned this Jan 17, 2022
@github-actions github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels Jan 17, 2022
@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Jan 17, 2022 •
edited
Loading

In v0.45, there is no breaking change now. The only breaking change I'm proposing is to:

testutil/known_values.go imho should be in testutil/testdata package.

But... it would be good to do the same changes in 0.44 - but there we can't touch known_values.go

@robert-zaremba robert-zaremba mentioned this pull request Jan 17, 2022
Merged
19 tasks
@alexanderbez
Copy link
Contributor

Agree with @robert-zaremba here 👍

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 18, 2022
@mergify mergify bot merged commit 285db06 into master Jan 18, 2022
@mergify mergify bot deleted the robert/generatecoinkey-master branch January 18, 2022 15:24
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@robert-zaremba
chore: move server.GenerateCoinKey and server.GenerateSaveCoinKey to … (
5ee07f3 
cosmos#10957)

Cleaning it and moving the breaking "test" code to testutils
Related to: cosmos#10956


---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaury1093 amaury1093 amaury1093 approved these changes

@anilcse anilcse anilcse approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@technicallyty technicallyty Awaiting requested review from technicallyty

@ryanchristo ryanchristo Awaiting requested review from ryanchristo

@aleem1314 aleem1314 Awaiting requested review from aleem1314

Assignees

@amaury1093 amaury1093

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Keys Keybase, KMS and HSMs T: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@robert-zaremba @amaury1093 @alexanderbez @anilcse