search

KomodoPlatform / komodo-defi-framework

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

Enable/electrum/my_balance 2.0 specification/ideas. #1006

Open artemii235 opened 3 years ago

artemii235 commented 3 years ago

@Milerius @sergeyboyko0791 @yurii-khi @shamardy @tonymorony If you have more ideas of what will be useful in the new coin activation RPC, please feel free to share :slightly_smiling_face:

Milerius commented 3 years ago
  • [ ] Add with_tokens arg to activate BNB/ETH/BCH with all tokens at once batching the requests to coin RPCs.
  • [ ] Return multiple addresses in the response. E.g. both legacy/segwit for UTXO at the same time. It will be also useful for HD wallet support.
  • [ ] Leave only enable call and add mode argument whenever applicable (native/electrum/zlite/etc.).
  • [ ] Allow passing all the configuration in the RPC instead of searching for it in the coins file.
  • [ ] Return the entire coin configuration in the response.

@Milerius @sergeyboyko0791 @yurii-khi @shamardy @tonymorony If you have more ideas of what will be useful in the new coin activation RPC, please feel free to share 🙂

I would like an option to integrate balance answer too, would be nice for GUI (right now i enable first and then i call my_balance for each coin)

artemii235 commented 3 years ago

@Milerius Sure, will add the same for my_balance 2.0 too, thanks!

shamardy commented 3 years ago

@Milerius @artemii235 I think in my_balance 2.0 for multiple address coins E.g. legacy/segwit it's good to have every address balance separately in addition to the total balance in the response.

Milerius commented 3 years ago

@Milerius @artemii235 I think in my_balance 2.0 for multiple address coins E.g. legacy/segwit it's good to have every address balance separately in addition to the total balance in the response.

Yeah but i was thinking about having the balance in the enable answer in order to have faster bootstrap in GUI (through an option let's say)

cipig commented 3 years ago

is what i get when i enable XRP-BEP20... the balance is there {"result":"success","address":"0x98298409c949135eeD89233D04C2cFef984baFF5","balance":"100.834683010699375075","unspendable_balance":"0","coin":"XRP-BEP20","required_confirmations":3,"requires_notarization":false}

Milerius commented 3 years ago

is what i get when i enable XRP-BEP20... the balance is there {"result":"success","address":"0x98298409c949135eeD89233D04C2cFef984baFF5","balance":"100.834683010699375075","unspendable_balance":"0","coin":"XRP-BEP20","required_confirmations":3,"requires_notarization":false}

Oh yes you are right, i think i can rly adjust my code when i enable then... will rework that when i've sometimes

tonymorony commented 3 years ago

I think some "scan" mode of activation might be helpful [ https://github.com/KomodoPlatform/AtomicDEX-mobile/issues/785 ] when API trying to activate coins from the list and remain active if the balance is greater than 0

Real UX problem for me to find coins I have balances for after seed restore. If implement such functionality it would be great to return scan progress (e.g. 1/100 coins checked 1% ... 50/100 coins checked 50%)

cf: https://github.com/KomodoPlatform/atomicDEX-API/issues/665

artemii235 commented 3 years ago

@tonymorony thanks for your comment! The "scan" mode is possible surely - I think it should be made as an additional RPC call because "enable" can become over-complicated. I've created the new issue for this: https://github.com/KomodoPlatform/atomicDEX-API/issues/1016

smk762 commented 3 years ago

Please incorporate https://github.com/KomodoPlatform/atomicDEX-API/issues/720 in this too

artemii235 commented 2 years ago

After working some time on this task I understood that it might be quite complicated to have the same API method for all coins types activation. Each protocol can have specific request parameters and errors, which will make the code and documentation quite bloated. @tonymorony @yurii-khi @Milerius What do you think about making a separate RPC for each protocol, e.g activate_utxo/activate_eth/...? Will this be convenient to work with in GUIs?

Milerius commented 2 years ago

After working some time on this task I understood that it might be quite complicated to have the same API method for all coins types activation. Each protocol can have specific request parameters and errors, which will make the code and documentation quite bloated. @tonymorony @yurii-khi @Milerius What do you think about making a separate RPC for each protocol, e.g activate_utxo/activate_eth/...? Will this be convenient to work with in GUIs?

For me there is no problem with that because I use an enum for coin_type - but I suggest activate_eth_family since we start to have lot of eth forks supported.

artemii235 commented 2 years ago

but I suggest activate_eth_family since we start to have lot of eth forks supported.

Family definition is not common in our codebase, activate_eth_protocol_coin will be more familiar :slightly_smiling_face:

smk762 commented 2 years ago

I'm in favour of as much separation as needed to avoid need to add conditional statements in to the docs like "if ERC, do this, if BEP, do that, else", but grouped as much as possible to common denominator.

From a user perspective, it might be simplest explained if each protocol has it's own activation method, even if its basically the same thing with a different name.

sergeyboyko0791 commented 2 years ago

it might be simplest explained if each protocol has it's own activation method

I'm afraid that if we want to add a new ETH fork, we would have to change the code.

artemii235 commented 2 years ago

it might be simplest explained if each protocol has it's own activation method

I'm afraid that if we want to add a new ETH fork, we would have to change the code.

I agree that this will be a bit of overkill - I plan to make activate/enable_erc20_protocol_coin for all the cases (ERC20/BEP20/...). @smk762 The request format will be the same for all token types - the only difference is the platform field in coins config. You can write it like ERC20/BEP20/... activation with a single request example.

artemii235 commented 2 years ago

Note for the future: refactoring is required for data structures used to keep information about activated tokens in platform coins. For now, we should either lock mutex for a long time, or use cloning that actually makes these mutexes "only write-safe". https://github.com/KomodoPlatform/atomicDEX-API/pull/1335#discussion_r933129239