Add Tether token support via ERC20 and Omni #4434
Conversation
boring-cyborg
bot
added
in:altcoins
is:no-priority
|
Thanks for opening this pull request! |
|
You need to add the assets to bisq.asset.Asset file. |
|
I looked into ways how to support the 2 flavors. I think easiest is to add special hanlding to PriceProvider.getAll method. Check for "USDT" result and add then 2 entries one with USDT-O and one with USDT-E, both same price. So there is a adjusted ticker entry for the price. Atm there are no prices but should be available. |
|
Tests are also required. |
What should be tested? There is no real new functionality. Or do you mean the tooltip string? |
I implemented this and amended my commit |
|
I suppose that just as in the USD/BTC market you see all offers, including multiple payment methods (Bank wire, Zelle, US Postal Money Order, etc)... you could have a single market for USDT and incluye variants as payment methods (L-USDT, USDT-O, USDT-E). That would be the ideal. However I would be happy to see a gradual approach in which we follow the path of least resistance and just add 1 as a regular altcoin (I would suggest USDT-Omni). IF there is demand/activity/volume we can improve on it in later releases. The advantages of a gradual approach with the minimum amount of work are several: -We increase the chances of having this for the next release. |
|
@flix1 do you suggest removing USDT-E from this PR and renaming USDT-O to USDT? |
|
@pingiun I don't really know how much work each of the options requires. It's your call. But if it's simpler, faster to just add one option as a regular altcoin... I would suggest USDT-O. That leaves the option open to add USDT-E in the future and creates the least amount of confusion. |
|
With the code in this PR I believe we can just enable USDT-O in the shipping client, leaving the code for the USDT-E token. I'm not sure how the tokens are filtered in the shipped client, does that work via the DAO asset listing? |
|
@flix1 That is not possible. The payment method for all altcoins is the same. That would be a major refactoring. If there is more need to group assets with diff. sub-flavors we could extend the Assets framework to support that. But I guess it first need to be proven that there is real demand. |
The addresses. See other assets. |
The addresses are already tested, as they are ethereum and bitcoin addresses |
|
For starters, the name of the coin isn't "Tether" but "Tether USD" which should be reflected in the class names, filenames, etc. in case we want to add Tether EUR or something in the future. Also, can you please add the Liquid Tether USD token under L-USDT ticker? As for the price feed, any new assets need to implement their own price feeds (if they want it) since we maintain our own price feeds now. |
pingiun
force-pushed
the
add-tether
branch
2 times, most recently
from
346079c
to
5545af3
Compare
Done, I guessed that L-USDT uses the same addresses as Liquid Bitcoin.
I will look at this in a new PR |
| addPrice(marketPriceMap, "USDT-E", price, timestampSec); | ||
| addPrice(marketPriceMap, "L-USDT", price, timestampSec); | ||
| } | ||
| addPrice(marketPriceMap, currencyCode, price, timestampSec); |
There was a problem hiding this comment.
We are adding USDT additionally as well. So we will end up with 4 price entries for USDT. I am not sure if L-USDT is treated as a seperate altcoin and if preice might differ slightly. I assume so. We should consider to either filter out the USDT-O and USDT-E in the price display code or to not add USDT (so users need to look up USDT-O or USDT-E). As both are the same price I guess its better to filter then out and use USDT.
There was a problem hiding this comment.
As this is getting more messy with sepcial cases I would suggest to add generic support. Add a new field in the asset to indicate the ticker currency for price feed (USDT). That will be used then for picking up price and price display in case the tickersymbol is not available in the price feed.
There was a problem hiding this comment.
On first glance this seems kind of hard because a lot of times currency codes are just passed around as strings instead of objects. But I'll investigate how I can make a nice solution
There was a problem hiding this comment.
I will have a look into it.
|
I looked into it and it is quite a larger effort not worth it as long we dont know if it will be really used anyway. |
|
Sorry could not make a PR to your branch somehow ;-( |
|
Closed in favour of #4457 |
Related: bisq-network/proposals#170, #2369
From the proposal and the discussion in Keybase it is clear that the USDT token is wanted on Bisq. This PR adds basic support for the ERC20 based token and the Omni based token.
I think a discussion should be had about the best way to make this implementation better. As of now, it adds two altcoins and thus two new markets are possible. I think it can be improved by joining these accounts or these markets in some way, for when users want to be able to trade both versions of USDT. On centralised markets there is only one USDT/XBT market, and the two tokens are considered equal. I think this behaviour is desirable.