search

pybitcash / bitcash

BitCash: Python Bitcoin Cash Library (fork of ofek's Bit)
https://bitcash.dev
MIT License
97 stars 39 forks source link

fixes sending cashtoken to leftover address #132

Closed yashasvi-ranawat closed 9 months ago

yashasvi-ranawat commented 9 months ago

During review, the key.address was reverted to cashaddress that doesn't signal cashtoken support. However, no unit tests caught the error when leftover address defaults to key.address. This implied that if a wallet had cashtoken and a leftover address wasn't specified, the leftover cashtokens were not sent and failed every transaction.

This pull request fixes this, and adds a test for the case.

yashasvi-ranawat commented 9 months ago

yes, both will work! The problem arises when you send all, but don't specify leftover as cashtoken address.

key.send([])

This fails because the leftover defaults to key.address! and cashtokens cannot be sent to an address that doesn't signal cashtoken.

merc1er commented 9 months ago

This fails because the leftover defaults to key.address! and cashtokens cannot be sent to an address that doesn't signal cashtoken.

Isn't that normal? To avoid sending to a non cashtoken address and potentially lose the funds?

yashasvi-ranawat commented 9 months ago

Isn't that normal? To avoid sending to a non cashtoken address and potential lose the funds?

I recon that if the leftover is not specified, that implies the leftover will come back to the wallet. And bitcash wallets do support and signal cashtokens. It is just that in the leftover, it uses non cashtoken signalling address, which is a bug.

However, if the user wants another leftover address, and that leftover doesn't signal cashtoken, then an error will rightfully be raised.

yashasvi-ranawat commented 9 months ago

potential lose the funds?

Just to clarify, the cashtoken implementation is different than SLP. If a cashtoken unaware wallet receives cashtokens, then either:

  1. it'll disregard those UTXO, because the script doesn't match.
  2. It'll raise errors because it won't be able to spend those UTXO's.

It can't accidentally burn them because the transaction pre-image is very different.

merc1er commented 9 months ago

I recon that if the leftover is not specified, that implies the leftover will come back to the wallet. And bitcash wallets do support and signal cashtokens. It is just that in the leftover, it uses non cashtoken signalling address, which is a bug.

Ah right, that makes sense!

And thanks for explaining the details of CashToken, which I am not familiar with. It's good to know funds can't be burnt accidentally like this, definitely an improvement over SLP!!

yashasvi-ranawat commented 9 months ago

it did not run in GitHub Actions, you might need to enable it on your fork's settings for future PRs

The run_tests workflow also has coverage upload to Codecov in them. The tests in my fork are successful in the run tests part but not upload to Codecov part. Any fork won't have access to the secrets that bitcash uses for the same. Maybe we can split the coverage upload and the testing parts? Or maybe there's another solution to it, that I don't know.

merc1er commented 9 months ago

Ah yes, I see the failed codecov step in your fork. That makes sense it doesn't have access to the secrets...

Not really something important, but I'll update with something like: https://github.com/marketplace/actions/python-coverage that doesn't require an external service...