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

UNITS_PER_CURRENCY to be fetched from db #1054

Closed
kilrau opened this issue Jun 24, 2019 · 2 comments · Fixed by #1904
Closed

UNITS_PER_CURRENCY to be fetched from db #1054

kilrau opened this issue Jun 24, 2019 · 2 comments · Fixed by #1904
Assignees
@sangaman
Labels
bug Something isn't working P1 top priority
Milestone
1.3.1

Comments

@kilrau
Copy link
Contributor

kilrau commented Jun 24, 2019

Reported by @offerm :

Trying to swap LTC/DAI. Failed.
I see this in the log

23/06/2019 13:59:15.460 [SWAPS] debug: New deal: {"takerCltvDelta":576,"rHash":"4f6572529ebf0b858c2458280a459e7e7dff023af30269037083fb2c4797f4f5","orderId":"0b6b3be0-95bf-11e9-9818-8f149c949153","pairId":"LTC/DAI","proposedQuantity":10000000,"rPreimage":"431bb884c5dde90d20d5a664a01ccea41b74dfff7b39ffdd57a1c272bd9d0cfe","takerCurrency":"LTC","makerCurrency":"DAI","takerAmount":10000000,"makerAmount":null,"destination":"0x7ed0299Fa1ADA71D10536B866231D447cDFa48b9","peerPubKey":"02b66438730d1fcdf4a4ae5d3d73e847a272f160fee2938e132b52cab0a0d9cfc6","localId":"16213d00-95bf-11e9-976d-eb69bb9b1bb9","price":86.1,"isBuy":false,"phase":0,"state":0,"role":0,"createTime":1561298355457}

note the makerAmount":null
diving into the code I found this:

Swaps.UNITS_PER_CURRENCY = {
    BTC: 1,
    LTC: 1,
    WETH: 10 ** 10,
};

Clearly DAI is missing. Also addcurrency can't be really used like that.
Not sure I understand why WETH has 10**10 units per currency.
@kilrau kilrau added P1 top priority critical bug labels Jun 24, 2019
@kilrau kilrau changed the title LTC/DAI swap failed LTC/DAI swap failed (+ UNITS_PER_CURRENCY need to be fetched from db) Jun 24, 2019
@ghost ghost mentioned this issue Jun 24, 2019
Merged
@ghost
Copy link

ghost commented Jun 24, 2019

I added #1058 as a quick fix. A more permanent/dynamic solution would be to fetch these values from the database, but I think we can lower the priority of this to after testnet/before mainnet.

@kilrau kilrau added bug Something isn't working and removed P1 top priority critical bug labels Jun 24, 2019
@kilrau kilrau assigned ghost and unassigned sangaman Jun 24, 2019
@sangaman sangaman mentioned this issue Jul 5, 2019
Merged
@kilrau kilrau added the P2 mid priority label Oct 9, 2019
@kilrau kilrau changed the title LTC/DAI swap failed (+ UNITS_PER_CURRENCY need to be fetched from db) UNITS_PER_CURRENCY to be fetched from db Oct 9, 2019
@kilrau kilrau mentioned this issue Apr 13, 2020
Closed
@kilrau kilrau added P3 low priority and removed P2 mid priority labels May 5, 2020
@kilrau kilrau closed this as completed Oct 19, 2020
@kilrau kilrau reopened this Nov 2, 2020
@kilrau kilrau assigned LePremierHomme and unassigned ghost and moshababo Nov 2, 2020
@kilrau kilrau added P1 top priority and removed P3 low priority labels Nov 2, 2020
@kilrau
Copy link
Contributor Author

kilrau commented Nov 2, 2020 •
edited
Loading

This issue is about removing https://github.com/ExchangeUnion/xud/blob/master/lib/utils/UnitConverter.ts#L1 and replacing it with https://github.com/ExchangeUnion/xud/blob/master/lib/utils/UnitConverter.ts#L42 to avoid any hardcoded decimals.

Bumping prio here since this should be the last piece missing to close #1888

This was referenced Nov 2, 2020
Merged
Merged
@kilrau kilrau assigned sangaman and unassigned LePremierHomme Nov 2, 2020
@kilrau kilrau added this to the 1.3.1 milestone Nov 6, 2020
sangaman added a commit that referenced this issue Dec 8, 2020
@sangaman
feat: bigint units & decimalplaces from db
d12cafc 
This commit makes two significant changes.

1. It uses the new `bigint` type - supported as of node LTS v14 - for
internal representations of the base "units" of any currencies/tokens.
ETH wei, having 10^18 units per ether, can exceed the capacity of
javascript's `number` type and lead to unexpected rounding or scientific
notation when converting to string that has required hacky workarounds.
Using `bigint` gives us native integer support for base tokens for all
tokens, regardless of how many decimal places they can be split into.

2. It reads the aforementioned decimal places from the database for
every token. Previously, we hardcoded the units per tokens for a handful
of known ERC20 tokens. Adding other currencies to be supported by xud
would have required a code change. Now, new tokens can be supported
with changes to the database via rpc calls, without requiring code
updates.

Closes #1054. Closes #1888.
sangaman added a commit that referenced this issue Dec 11, 2020
@sangaman
feat: bigint units & decimalplaces from db
7058a15 
This commit makes two significant changes.

1. It uses the new `bigint` type - supported as of node LTS v14 - for
internal representations of the base "units" of any currencies/tokens.
ETH wei, having 10^18 units per ether, can exceed the capacity of
javascript's `number` type and lead to unexpected rounding or scientific
notation when converting to string that has required hacky workarounds.
Using `bigint` gives us native integer support for base tokens for all
tokens, regardless of how many decimal places they can be split into.

2. It reads the aforementioned decimal places from the database for
every token. Previously, we hardcoded the units per tokens for a handful
of known ERC20 tokens. Adding other currencies to be supported by xud
would have required a code change. Now, new tokens can be supported
with changes to the database via rpc calls, without requiring code
updates.

Closes #1054. Closes #1888.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sangaman sangaman

Labels
bug Something isn't working P1 top priority
Projects
None yet
Milestone
1.3.1
Development

Successfully merging a pull request may close this issue.

feat: bigint units & decimalplaces from db ExchangeUnion/xud
4 participants
@sangaman @moshababo @kilrau @LePremierHomme