feat: add NIM - CRC swap #502
Conversation
onmax
force-pushed
the
onmax/sinpe
branch
3 times, most recently
from
56e493b
to
adcac24
Compare
There was a problem hiding this comment.
Some things which I think should be improved:
- There's the assumption in many places in the code that only one side of the swap is a fiat currency. This should be enforced by the request parser.
- I don't think that fractional CRC amounts should be displayed.
- Quite a lot of code repetition / copy-paste between
EURandCRChandling, which can in most cases be reasonably well be generalized and merged. - Some code style suggestions.
I think at a minimum, the stricter parsing and handling of fractional CRC amounts should be addressed, so I'm requesting changes.
src/components/SwapFeesTooltip.js
Outdated
| ? redeemTx.fee | ||
| : 0; | ||
|
|
||
| const theirFee = fundTx.type === 'EUR' ? fundFees.processing : redeemFees.processing; | ||
| const theirFee = fundTx.type === 'EUR' || fundTx.type === 'CRC' | ||
| ? fundFees.processing : redeemFees.processing; |
There was a problem hiding this comment.
| ? fundFees.processing : redeemFees.processing; | |
| ? fundFees.processing | |
| : redeemFees.processing; |
| @@ -0,0 +1,3 @@ | |||
| const CrcConstants = { // eslint-disable-line no-unused-vars | |||
| CENTS_PER_COIN: 100, | |||
There was a problem hiding this comment.
Historically, CRC did in fact have cents but those are not in circulation anymore. Are the amounts in request types in CRC cents or CRC? Until seeing this line, I assumed CRC, but in any case, a comment should be added in PublicRequest to clarify this.
Are fractional CRC amounts also displayed in the UI? In that case, I don't think we should do so, except for exchange rates.
There was a problem hiding this comment.
When we added the wallet exchange logic, all Oasis fiat was in EUR. Now the wallet uses multiple / 100 conversions from cents to coins. See here for an example.
To keep things simple in the Wallet PR, I used 2 decimal places for the CRC, which Jeff confirmed is valid, though less common. This gives us more precision and is still a correct format while keeping the PR as simple as possible without breaking changes for EUR swaps.
Let me know your thoughts on this trade-off.
| /* global CrcConstants */ | ||
|
|
||
| class CrcUtils { // eslint-disable-line no-unused-vars | ||
| /** | ||
| * @param {number} coins CRC amount in decimal | ||
| * @returns {number} Number of CRC cents | ||
| */ | ||
| static coinsToCents(coins) { | ||
| return Math.round(coins * CrcConstants.CENTS_PER_COIN); | ||
| } | ||
|
|
||
| /** | ||
| * @param {number} cents Number of CRC cents | ||
| * @returns {number} CRC count in decimal | ||
| */ | ||
| static centsToCoins(cents) { | ||
| return cents / CrcConstants.CENTS_PER_COIN; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think, these should be combined with EuroUtils into FiatUtils, in a similar fashion as the CryptoUtils, which are also not individual utils for each crypto asset, or if they are, they provide more utilities specific to that crypto.
There was a problem hiding this comment.
We already have all the assets under CryptoUtils (both fiat and crypto). Maybe the CryptoUtils name is not the best, but renaming the class should be another PR?
|
Another question: the commit history refers to NIM-CRC swaps, but in the code I didn't see anything that specifically disallows any other CRC swaps, e.g. BTC-CRC. Are those possible, or should they be disallowed by the |
No description provided.