KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
107 stars 94 forks source link

my_balance and orderbook shows old address format for BCH #466

Closed cipig closed 4 years ago

cipig commented 5 years ago

the calls my_balance and orderbook are showing the old address format for BCH "address" : "1JsAjr6d21j9T8EMsYnQ6GXf1mM523JAv1"

https://blockchair.com/bitcoin-cash/address/1JsAjr6d21j9T8EMsYnQ6GXf1mM523JAv1 shows

Legacy address format 1JsAjr6d21j9T8EMsYnQ6GXf1mM523JAv1
Cash address format qrplwyx7kueqkrh6dmd3fclta6u32hafp5tnpkchx2

mm2 should use the "Cash address format"

libercash commented 5 years ago

One more problem arised using BCH on AtomicDEX. I'm posting it here instead of opening another issue, I don't know if I'm acting correctly. One user tried to withdrawal BCH fund from AtomicDEX to another BCH wallet. By default, atomicDEX uses a fix 1000 satoshis fees, but the tx was large (lots of inputs) so miner fee was finally ~0.7 sat/B. The transaction is unconfirmed until Bitcoin.com mines it, is the only pool I know mining < 1sat/B txs.

AtomicDEX must be able to set a fee of 1 sat/B to avoid this in the future.

tonymorony commented 4 years ago

Just a little bump - there is a demand for cash address format usage so it's better to switch to it if it's not hard

https://discordapp.com/channels/412898016371015680/584762059430821909/711788712694120460 https://discordapp.com/channels/412898016371015680/705374203972419616/712222902619078657

artemii235 commented 4 years ago

We should also allow withdraw to addresses encoded in this format.

sergeyboyko0791 commented 4 years ago

@tonymorony, Could you please check the changes https://github.com/KomodoPlatform/atomicDEX-API/pull/666

To enable cashaddress BCH address format, add the following to the init call: {"coin":"BCH","asset":"BCH","required_confirmations":0,"address_format":{"format":"cashaddress","network":"bchtest"}}, Available network prefixes: bitcoincash (Main net), bchtest (Test net), bchreg.

To enable standard UTXO address format, remove the address_format field or replace "address_format":{"format":"cashaddress","network":"bchtest"} to "address_format":{"format":"standard"}

What should be tested:

tonymorony commented 4 years ago

upd cleared my confusion, such entry in coins file worked (missed comma at first):

        {
                "coin": "BCH",
                "name": "bch",
                "fname": "Bitcoin Cash",
                "rpcport": 33333,
                "pubtype": 0,
                "p2shtype": 5,
                "wiftype": 128,
                "txfee": 1000,
                "segwit": true,
                "mm2": 1,
                "required_confirmations": 1,
                "address_format":{"format":"cashaddress","network":"bchreg"}
        },

@sergeyboyko0791 I'm a little bit confused if I'm doing everything correct. is init == electrum call?. I've tried different combinations of format/network in electrum call but getting legacy address. I've tried to add "address_format":{"format":"cashaddress","network":"bchtest"} field for BCH to coins file also but such construction not parsing on startup

curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"electrum\",\"coin\":\"BCH\",\"servers\":[{\"url\":\"bch.imaginary.cash:50001\"},{\"url\":\"bch0.kister.net:50001\"}],\"address_format\":{\"format\":\"cashaddress\",\"network\":\"bchreg\"}}" | jq
{
  "address": "1FjaqgbLPtiRMvrw1WuXrVpHyXo8sBg5Gq",
  "balance": "0",
  "coin": "BCH",
  "locked_by_swaps": "0",
  "required_confirmations": 1,
  "requires_notarization": false,
  "result": "success"
}
curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"electrum\",\"coin\":\"BCH\",\"servers\":[{\"url\":\"bch.imaginary.cash:50001\"},{\"url\":\"bch0.kister.net:50001\"}],\"address_format\":{\"format\":\"cashaddress\",\"network\":\"bchtest\"}}" | jq
{
  "address": "1FjaqgbLPtiRMvrw1WuXrVpHyXo8sBg5Gq",
  "balance": "0",
  "coin": "BCH",
  "locked_by_swaps": "0",
  "required_confirmations": 1,
  "requires_notarization": false,
  "result": "success"
}

btw random phrase as network worked as well:

curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"electrum\",\"coin\":\"BCH\",\"servers\":[{\"url\":\"bch.imaginary.cash:50001\"},{\"url\":\"bch0.kister.net:50001\"}],\"address_format\":{\"format\":\"standard\",\"network\":\"mynameistonyl\"}}" | jq
{
  "address": "1FjaqgbLPtiRMvrw1WuXrVpHyXo8sBg5Gq",
  "balance": "0",
  "coin": "BCH",
  "locked_by_swaps": "0",
  "required_confirmations": 1,
  "requires_notarization": false,
  "result": "success"
}
tonymorony commented 4 years ago

It looks like address conversion works not correct/not as expected. In mm2 I'm getting

legacy format:

curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"electrum\",\"coin\":\"BCH\",\"servers\":[{\"url\":\"bch.imaginary.cash:50001\"},{\"url\":\"bch0.kister.net:50001\"}]\" | jq
{
  "address": "1FjaqgbLPtiRMvrw1WuXrVpHyXo8sBg5Gq",
  "balance": "0",
  "coin": "BCH",
  "locked_by_swaps": "0",
  "required_confirmations": 1,
  "requires_notarization": false,
  "result": "success"
}

usual:

 curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"electrum\",\"coin\":\"BCH\",\"servers\":[{\"url\":\"bch.imaginary.cash:50001\"},{\"url\":\"bch0.kister.net:50001\"}]}" | jq
{
  "address": "bchreg:qzse774z2nww6zshjg9trs4v76vgg8m87c2pg99nc5",
  "balance": "0",
  "coin": "BCH",
  "locked_by_swaps": "0",
  "required_confirmations": 1,
  "requires_notarization": false,
  "result": "success"
}

address which I'm getting for 1FjaqgbLPtiRMvrw1WuXrVpHyXo8sBg5Gq legacy address not matching expected one (bitcoincash:qzse774z2nww6zshjg9trs4v76vgg8m87c506ryhuw) which I'm getting from https://bch.btc.com/tools/address-converter https://cashaddr.bitcoincash.org/ https://bitpay.github.io/address-translator/ converters

cipig commented 4 years ago

hmmm, works for me so far

used this in coins file:

        {
                "coin": "BCH",
                "name": "bch",
                "fname": "Bitcoin Cash",
                "rpcport": 33333,
                "pubtype": 0,
                "p2shtype": 5,
                "wiftype": 128,
                "txfee": 1000,
                "segwit": true,
                "mm2": 1,
                "address_format": {"format":"cashaddress","network":"bitcoincash"},
                "required_confirmations": 1
        },

and electrum shows this: {"address":"bitcoincash:qrk4duh7hyze8z696tsqnmq54hgw05q4wg6xq0ssee","balance":"0","coin":"BCH","locked_by_swaps":"0","required_confirmations":1,"requires_notarization":false,"result":"success"}, which is the correct bch-address of this node: https://blockchair.com/bitcoin-cash/address/1NdwJoQVLxj5kCHXKcaLxWrByddbgyHofb

orderbook shows

  "asks": [
    {
      "coin": "BCH",
      "address": "bitcoincash:qrplwyx7kueqkrh6dmd3fclta6u32hafp5tnpkchx2",
      "price": "351.88723957",
      "maxvolume": "3.89898",
      "pubkey": "1bb83b58ec130e28e0a6d5d2acf2eb01b0d3f1670e021d47d31db8a858219da8",
      "age": 4,
      "zcredits": 0,
      "uuid": "3b5db41d-92da-48ef-aa2f-7d501836ffb5"
    },
    {
      "coin": "BCH",
      "address": "bitcoincash:qqfykzzxygl00qfshrj5fwd0cwcfnzprscfu3xe658",
      "price": "348.4708586",
      "maxvolume": "0.63175491",
      "pubkey": "dbd8c73e2e80e4f3cf88d2f04a9d2d0df4269496608b14a3e17556fdcb01e0c1",
      "age": 6,
      "zcredits": 0,
      "uuid": "2b4a2619-8f29-4c11-80ab-bfe45d0dc827"
    }

and i checked and the addresses shown are also correct

tonymorony commented 4 years ago

wow, very interesting @cipig that it works correct for your address and not correct for mine not sure if it makes any difference but I'm using KMD wif starting from U, not the seed. will try with seed now

cipig commented 4 years ago

you used "network":"bchreg", i used "network":"bitcoincash"

tonymorony commented 4 years ago

@cipig right, that was my confusion. I've thought by some reason that bchreg should mean something like regular, not regtest 😖 tyvm for help

cipig commented 4 years ago

looks good, swaps working between old makers and new cashaddr and legacy takers, withdraw works too

withdraw with cashaddr to legacy addr gives error: "error" : "rpc:303] lp_coins:659] utxo:1403] cashaddress contains mixed upper and lowercase characters" and withdraw with legacy addr to cashaddr gives error: "error" : "rpc:303] lp_coins:659] utxo:1402] Invalid Address"

tonymorony commented 4 years ago

Swap with old maker, transactions and balance getting works fine too on my side 👍

Outstading things to deliever this feature to end users are PR information about these mods into docs, into coins file (with cashaddress as default option), and GUIs update then. Opened related issues in GUIs repos

sergeyboyko0791 commented 4 years ago

@cipig

withdraw with cashaddr to legacy addr gives error:

I considered to not allow user uses legacy and cashaddress address formats at the same time. It was made to make the code more readable and to achieve unambiguous behavior according to the selected settings.

cipig commented 4 years ago

everything works as expected, thanks a lot coins file now has cashaddr as default: https://github.com/KomodoPlatform/coins/commit/fdb62eb0741be071f773a4a259cee7d1d7f9dac1 and i use it on all the maker nodes docs update: https://github.com/KomodoPlatform/coins/pull/17