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

R4R: Add custom validation for denom #6755

Merged
merged 13 commits into from
Oct 6, 2020

Conversation

okwme
Copy link
Contributor

@okwme okwme commented Jul 17, 2020

Description

Allow ValidateDenom to be customized per application.

closes: #6744


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

@okwme
Copy link
Contributor Author

okwme commented Jul 17, 2020

@alessio I added the technique you described in #6744 but I'm still unsure how as an app developer I would utilize this change. When I add modules to my app.go is it possible to somehow overwrite the ValidateDenom with a new function? This wouldn't necessarily be honored by other modules which import the Coin type though right? Or does this need a more complicated architecture where the validation rules are stored somewhere else?

@okwme okwme changed the title Add custom validation for denom WIP: Add custom validation for denom Jul 17, 2020
@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #6755 into master will decrease coverage by 0.25%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #6755      +/-   ##
==========================================
- Coverage   55.50%   55.24%   -0.26%     
==========================================
  Files         440      591     +151     
  Lines       29169    36949    +7780     
==========================================
+ Hits        16190    20414    +4224     
- Misses      11352    14427    +3075     
- Partials     1627     2108     +481     

@fedekunze fedekunze marked this pull request as draft July 17, 2020 08:42
@colin-axner
Copy link
Contributor

This is unrelated to your question above, but with regards to IBC it would be cool if a custom denomination on chainA that is not supported by chainB could still be transferred to chainB without error. I think this should work fine if we represent denominations as hashes of the IBC path, assuming chainB supports hashes as denoms.

cc/ @cwgoes @fedekunze

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

👍

@fedekunze fedekunze mentioned this pull request Aug 13, 2020
4 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 1, 2020
@okwme okwme removed the stale label Sep 1, 2020
@okwme
Copy link
Contributor Author

okwme commented Sep 1, 2020

sorry, making this a priority this week

@okwme okwme mentioned this pull request Oct 4, 2020
9 tasks
@okwme okwme changed the title WIP: Add custom validation for denom R4R: Add custom validation for denom Oct 4, 2020
@okwme
Copy link
Contributor Author

okwme commented Oct 4, 2020

Would love some help understanding why test coverage is failing.
@marbar3778 @fedekunze any ideas?

@okwme okwme changed the title R4R: Add custom validation for denom WIP: Add custom validation for denom Oct 4, 2020
@alessio
Copy link
Contributor

alessio commented Oct 4, 2020

Would love some help understanding why test coverage is failing.

@okwme 'cause codecov.io sucks in so many different ways. I will review again when it's ready, though so far code looks good, test cases seem sufficient. Don't bother please and keep up the good work!

@tac0turtle
Copy link
Member

tac0turtle commented Oct 4, 2020

Would love some help understanding why test coverage is failing.

seems to be missing some values:

# github.com/cosmos/cosmos-sdk/types_test [github.com/cosmos/cosmos-sdk/types.test]
types/coin_test.go:104:2: undefined: CoinDenomRegex
types/coin_test.go:109:14: undefined: Coin
types/coin_test.go:112:4: undefined: Coin
types/coin_test.go:113:4: undefined: Coin
types/coin_test.go:114:4: undefined: Coin
types/coin_test.go:115:4: undefined: Coin
types/coin_test.go:116:4: undefined: Coin
types/coin_test.go:120:3: undefined: require
types/coin_test.go:122:2: undefined: CoinDenomRegex
types/coin_test.go:122:19: undefined: DefaultCoinDenomRegex
types/coin_test.go:122:19: too many errors

does this not happen when running tests locally?

@okwme
Copy link
Contributor Author

okwme commented Oct 5, 2020

@marbar3778 oops the test format totally changed from v0.38. Passing now.

@okwme okwme changed the title WIP: Add custom validation for denom R4R: Add custom validation for denom Oct 6, 2020
@okwme okwme marked this pull request as ready for review October 6, 2020 13:27
@@ -163,6 +163,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa

### Features

* [\#6755](https://github.com/cosmos/cosmos-sdk/pull/6755) Add custom regex validation for `Coin` denom by overwriting `CoinDenomRegex` when using `/types/coin.go`.
Copy link
Contributor

Choose a reason for hiding this comment

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

@okwme this should be moved across the the Launchpad branch as 0.39.2 will be the first release where this new feature is introduced.

@alessio alessio merged commit 3e6089d into master Oct 6, 2020
@alessio alessio deleted the okwme/6744-custom-denom-validation branch October 6, 2020 17:06
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.

5 participants