[fix] #296 : Prevent sending transaction with 0 as amounts

Fix issue with --allSources creating a transaction when balance is 0. Tests will be written when #213 (closed) is merged.

Close #296 (closed).

added D: Transfer enhancement labels

changed milestone to %0.9.0

@matograine, would you like to be assigned to review this change?

@moul I first thought about implementing it through Click. However from what I understand --allSources is a flag option that sends all the sources from the sender's pubkey to the recipient. So I inferred that in order to check whether the amount is 0 we first need to get this amount, which happens after the arguments are parsed by Click.

However, --amount is handled through Click whereas --amountUD is handled here.

I guess it would be more efficient to handle --amountUD through Click (which I can do quickly), but I don't see how I could possibly do it for --allSources.

Did I get it wrong ?

Good catch, in fact there is also an issue with --allSources when the balance of the pubkey is null. So, when:

• --allSources is passed, check if the balance is positive. Click can't be used in this case as far as I can tell.
• --amountUD is passed, filter with Click small amounts, but still let small amount pass. I would set the minimum value to 0.001 for Ğ1 currency. This value can be lowered to 0.00001 to allow smaller tx on Ğ1-test.
• --amount is already handled

assigned to @matograine

added 1 commit

• 85106300 - [fix] #296 (closed) : Show issuer pubkey:checksum on error

Compare with previous version

added 1 commit

• 14463a61 - [fix] #296 (closed) : fix code formatting

Compare with previous version

Nice !

• don't hesitate to use git push --force, so all little changes (formats, etc.) are in the same commit. I think all your changes should fit in one only commit. (if you disagree, just tell me).

• I don't know if you installed pre-commit. I recommend it for next developments, it will prevent such little formatting problems.

• you can re-launch tests for Py-v3.9 on your pipeline. Tests frequently fail, we just have to re-launch them :-)

Once all this is done, I think I can approve.

added 1 commit

• f0337a49 - [fix] #296 (closed) : Prevent sending transaction with 0 as amounts

Compare with previous version

added 1 commit

• 82ff5a71 - [fix] #296 (closed) : Prevent sending transaction with 0 as amounts

Compare with previous version

added 1 commit

• 5d696df8 - [fix] #296 (closed) : Prevent sending transaction with 0 as amounts

Compare with previous version

The : should be shown. Maybe the user only made a mistake in the credentials. Hint : you may use display_pubkey_and_checksum()

Done

don't hesitate to use git push --force, so all little changes (formats, etc.) are in the same commit. I think all your changes should fit in one only commit. (if you disagree, just tell me).

I usually don't use --force but if it's ok in your workflow I can use it :)

I don't know if you installed pre-commit. I recommend it for next developments, it will prevent such little formatting problems.

Will do soon-ish

you can re-launch tests for Py-v3.9 on your pipeline. Tests frequently fail, we just have to re-launch them :-)

WIP

Can you use the f-string feature? Regarding the text, I would replace "0" by "null" or say the "pubkey is empty"

Done. I prefer empty over null, the former sounding more user-friendly to me than the latter. Is it ok for you ?

Can you check when <= 0 to be on the safe side.

Done

Done. I prefer empty over null, the former sounding more user-friendly to me than the latter. Is it ok for you ?

As you like. I am fine with both. I prefer the empty one. Thanks.

added 1 commit

• cfb70c8a - [fix] #296 (closed) : Prevent sending transaction with 0 as amounts

Compare with previous version

added 2 commits

• d24ef804 - [fix] #296 (closed) : Prevent sending 0 with --allSources
• a4510d2d - [fix] #296 (closed) : Prevent sending 0 with --amountUD

Compare with previous version

resolved all threads

changed the description

resolved all threads

The code looks good to me. Great work! Waiting for the tests. I will try to finish my review on #213 (closed).

The code looks good to me

It still needs a rebase on current dev branch (@moul added changes, and I corrected a broken integration test)

Waiting for the tests.

I would rather write them in !130 (merged) so the style and values are coherent.

So I propose we merge it right after the rebase.

added 7 commits

• a4510d2d...bcb0f121 - 5 commits from branch clients/python:dev
• 43fec2fa - [fix] #296 (closed) : Prevent sending 0 with --allSources
• bda2b8cd - [fix] #296 (closed) : Prevent sending 0 with --amountUD

Compare with previous version

It still needs a rebase on current dev branch (@moul added changes, and I corrected a broken integration test)

Done. Waiting for the tests to pass.

approved this merge request

approved this merge request

merged

mentioned in issue #362 (closed)

mentioned in merge request !130 (merged)