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

feat: report channel details in correct asset scale #50

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@condaatje
Copy link

commented Mar 20, 2019

feat: report channel details in correct asset scale

Example: current network defaults use asset scale 9 (whereas drops are asset scale 6), so if I run ilp-spsp send --amount 1000000 --receiver '$xxx' my resulting balance will be increased by 1,000 (not 1,000,000). At no point in the default workflow is this information conveyed to the user.

index  channel id   destination   amount (drops)    balance (drops)     expiry
0      xx...        xx...         10,000,000        21,000

In this PR I propose we reflect the user's configured asset scale in the moneyd xrp:info and similar reports to avoid such confusion. For example, an asset scale of 9 would result in the following display:

index  channel id   destination   amount (1E-9XRP)  balance (1E-9XRP)   expiry
0      xx...        xx...         10,000,000,000    21,000,000

One thing I would like to be sure of in review is that this does not behave improperly when running multiple moneyd instances with different config files (and different asset scales) on the same machine. Any other bits of feedback welcome too.

Also, the moneyd README says the command moneyd xrp:topup --amount 1000 will topup 1000 drops regardless of asset scale. This is a further source of confusion. However, xrp:topup appears to have no effect currently - so asset scale issues are probably not the most pressing. That being said, it's likely worth investigating which other areas of the code might be exhibiting the same confusing behaviour addressed in this PR.

If some are identified, I would be happy to add more fixes before merging.

@jsf-clabot

This comment has been minimized.

Copy link

commented Mar 20, 2019

CLA assistant check
All committers have signed the CLA.

@austin-king austin-king requested review from d1no007 and austin-king Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.