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
Choose a base branch
from
Open

feat: report channel details in correct asset scale #50

wants to merge 1 commit into from

Conversation

condaatje
Copy link

@condaatje condaatje 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
Copy link

jsf-clabot commented Mar 20, 2019

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants