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

[Discussion] Remove xucli addcurrency <currency> + removecurrency #896

Closed
kilrau opened this issue Apr 17, 2019 · 11 comments
Closed

[Discussion] Remove xucli addcurrency <currency> + removecurrency #896

kilrau opened this issue Apr 17, 2019 · 11 comments
Labels
command line (CLI) Relating to the command line interface tools enhancement New feature or request grpc gRPC API P3 low priority question/tbd Further information or discussion needed

Comments

@kilrau
Copy link
Contributor

kilrau commented Apr 17, 2019

Abstract: How about we get rid of xucli addcurrency <currency> and just add a currency automatically when xucli addpair <base_currency> <quote_currency> contains a new currency? The same way xucli removepair <pair_id> would remove the currency if it's not used anywhere else.

Motivation: As @offerm just discovered, it's not too user-friendly to think of the step of adding a currency first before one is able to addpair for such. Others will too.

Pro: better UX, wouldn't need to change internals, wouldn't need a new listcurrencies call.
Alternative would be to get rid of the "currency" concept altogether.

PS: I would also want to straighten the the addpair call and command to:
xucli addpair <pair_id> add a trading pair in the format <base_currency>/<quote_currency> to be consistent with other <pair_id> commands. At the moment it's

xucli addpair btc ltc but
xucli removepair btc/ltc

Let's agree on sth first, not high prio obviously, see milestone.

@kilrau kilrau added enhancement New feature or request grpc gRPC API command line (CLI) Relating to the command line interface tools labels Apr 17, 2019
@kilrau kilrau added this to the 1.0.0-sprint.15 milestone Apr 17, 2019
@kilrau kilrau added the P2 mid priority label Apr 17, 2019
@kilrau kilrau modified the milestones: 1.0.0-sprint.15, 1.0.0-sprint.16 Apr 17, 2019
@offerm
Copy link
Contributor

offerm commented Apr 17, 2019

Agree to the concept.
For ERC20 we need to provide also the token address so we need to see how to provide this extra info.

@kilrau
Copy link
Contributor Author

kilrau commented Apr 17, 2019

Good point. Maybe:

xucli addpair <pair_id> [token addresses]
Description: add a trading pair in the format <base_currency>/<quote_currency> with additional [base_currency_token_address]/[quote_currency_token_address] for tokens.

Other calls can remain the same.

@erkarl
Copy link
Collaborator

erkarl commented Apr 17, 2019

I agree that skipping addcurrency would be a better UX for the end user, but we also need to provide the decimal_places and contract_address for ERC20 tokens. It feels like including those in the addpair call would make things even more complicated. Another option is to fetch this information from a local database/mapping (not sure about this either).

@kilrau
Copy link
Contributor Author

kilrau commented Apr 17, 2019

Outdated stats on erc20 decimals distribution: ethereum/EIPs#724 (comment), so not 9X% with 18 decimals like I thought it would be.

I'd indeed want to automate fetching all additional info of a token address:

  • How about fetching it from geth calling the decimals function of the given contract?
  • For that we would need an additional check that geth and raiden are actually available before we allow adding contract addresses. Which they should be anyways but maybe not in the moment of adding a pair. Opinions?

@offerm
Copy link
Contributor

offerm commented Apr 17, 2019

I think it is valid to request raiden when adding an ERC20 token. Don't see a problem with that.

Also, as far as I recall, Raiden used to provide token information in their token API. This was removed in one of the latest version. We can ask them to put it in so we can query raiden and not geth/parity/infura/etc.

But I'm not sure we should touch this at all.... My question was not about the "how to add a pair" but more about - what is still missing in xud before an order can be issued for WETH/BTC. The answer is - "nothing".

My question now - what is missing in xud before a swap can be agreed between the maker and taker?

@sangaman
Copy link
Collaborator

We would also need to specify the swap client (between lnd and raiden for now). It seems more complicated and confusing to me to try to combine adding a currency and adding a trading pair into a single call. In terms of UX, this is something that has to be done rarely and only by administrators.

But I do agree that the cli command for add pair should use the ltc/btc format rather than ltc btc to be consistent.

@kilrau
Copy link
Contributor Author

kilrau commented Apr 18, 2019

Not crucial at the moment, just like I said.

We can go with a manual addcurrency call specifying all these details including swap client for now. In the long run I would want to get rid of "currencies" altogether and just have pairs once we are able to fetch all info from underlying clients.

@kilrau kilrau modified the milestones: 1.0.0-sprint.16, 1.0.0, post-1.0.0 May 8, 2019
@kilrau kilrau removed the P2 mid priority label May 8, 2019
@kilrau
Copy link
Contributor Author

kilrau commented May 8, 2019

ERC20 and also other token standards contain a name and symbol field. We could just read this directly from the full node (e.g. geth) and use it as ticker symbol for the currency instead of asking the user to enter it manually. User can confirm or alter this. Token addresses without name or symbol require the user to enter a ticker symbol name.

@offerm
Copy link
Contributor

offerm commented May 8, 2019 via email

@kilrau kilrau removed this from the post-1.0.0 milestone Jun 11, 2019
@kilrau kilrau added the P3 low priority label Oct 9, 2019
@engwarrior
Copy link
Contributor

Pls provide estimate for this task.

@kilrau kilrau added the question/tbd Further information or discussion needed label Feb 11, 2020
@kilrau
Copy link
Contributor Author

kilrau commented Feb 11, 2020

This one needs further discussion if that's even possible @engwarrior

@kilrau kilrau closed this as completed Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command line (CLI) Relating to the command line interface tools enhancement New feature or request grpc gRPC API P3 low priority question/tbd Further information or discussion needed
Projects
None yet
Development

No branches or pull requests

5 participants