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

Fix ledger custom coin type support bug #4716

Merged
merged 11 commits into from
Jul 29, 2019
Merged

Conversation

yun-yeo
Copy link
Contributor

@yun-yeo yun-yeo commented Jul 12, 2019

  • This PR will fix custom hrp usage and ledger test bug for custom coin type project.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #4716 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #4716      +/-   ##
==========================================
- Coverage   50.47%   50.46%   -0.01%     
==========================================
  Files         288      288              
  Lines       18571    18572       +1     
==========================================
  Hits         9373     9373              
- Misses       8509     8510       +1     
  Partials      689      689

@fedekunze fedekunze requested a review from jleni July 15, 2019 15:19
@fedekunze fedekunze added C:Ledger Issues and features related Ledger integration and functionality R4R labels Jul 15, 2019
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.

untested ACK

@jleni
Copy link
Member

jleni commented Jul 17, 2019

Would it be possible to add an additional test that uses this mock class with a different coin type and hrp?

@jleni
Copy link
Member

jleni commented Jul 17, 2019

I would not say this is actually a bug but when custom coin types were added, it seems that tests where not extended to support them.

It would be good to add test cases and test vectors too.

It should be important to clarify, that originally this mock was done specifically for the current ledger nano app for cosmos.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

blocking on the tests requested by @jleni

@fedekunze fedekunze added WIP and removed R4R labels Jul 18, 2019
@alexanderbez
Copy link
Contributor

Bump

@yun-yeo
Copy link
Contributor Author

yun-yeo commented Jul 23, 2019

@jleni I add some test code for the custom coin type.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, missing changelog entry. Please use clog add -i

@fedekunze fedekunze added R4R and removed WIP labels Jul 23, 2019
@yun-yeo
Copy link
Contributor Author

yun-yeo commented Jul 29, 2019

@fedekunze done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Ledger Issues and features related Ledger integration and functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants