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 ETH as a currency #618

Merged
merged 3 commits into from
Mar 16, 2020
Merged

add ETH as a currency #618

merged 3 commits into from
Mar 16, 2020

Conversation

nhartner
Copy link
Collaborator

No description provided.

Signed-off-by: nhartner <nhartner@gmail.com>
Signed-off-by: nhartner <nhartner@gmail.com>
nkramer44
nkramer44 previously approved these changes Mar 13, 2020
Copy link
Collaborator

@nkramer44 nkramer44 left a comment

Choose a reason for hiding this comment

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

LGTM, other than some copy/paste errors.

@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #618 into master will increase coverage by 0.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #618      +/-   ##
============================================
+ Coverage     65.76%   65.82%   +0.05%     
- Complexity     1149     1153       +4     
============================================
  Files           297      298       +1     
  Lines          5784     5796      +12     
  Branches        228      228              
============================================
+ Hits           3804     3815      +11     
  Misses         1879     1879              
- Partials        101      102       +1     
Impacted Files Coverage Δ Complexity Δ
...tor/javax/money/providers/EthCurrencyProvider.java 90.90% <90.90%> (ø) 3.00 <3.00> (?)
...ver/spring/settings/javamoney/JavaMoneyConfig.java 100.00% <100.00%> (ø) 18.00 <1.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32e4431...cf3d37c. Read the comment docs.

Signed-off-by: nhartner <nhartner@gmail.com>
@Test
public void getCurrency() {
assertThat(Monetary.getCurrency("XRP").getDefaultFractionDigits()).isEqualTo(3);
assertThat(Monetary.getCurrency("ETH").getDefaultFractionDigits()).isEqualTo(9);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not entirely sure if/where getDefaultFractionDigits is used in JavaMoney, if at all. I don't think we use this anywhere in our actual FX code, so maybe this is unimportant to the Connector.

That said, my sense is that this value is just for default display purposes. E.g., for USD and EUR, it's 2, but for JPY it's 0. This doesn't preclude a developer from using fractional numbers, so maybe this setting is not important. E.g., in the Javadoc, it says "In the case of pseudo-currencies, such as IMF Special Drawing Rights, -1 is returned."

All that to say, the CryptoCompare API seems to use 4 for XRP and 6 for Eth for display purposes (see here and enter https://min-api.cryptocompare.com/data/price?fsym=USD&tsyms=ETH).

So, maybe we set these to 4 and 6? Or maybe this has some other meaning that I'm missing?

@nhartner nhartner merged commit e1d5abe into master Mar 16, 2020
@nhartner nhartner deleted the nh/eth-currency-provider branch March 16, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants