Deactivation of platform coin also deactivates its tokens, for all platforms

[yurii-khi]

Not sure if bug of feature, but after #1493, disable_coin RPC for platform coin (e.g. BNB) also automatically deactivates all its tokens (e.g. KMD-BEP20). Looks like it work like this for all platforms/tokens, including ERC-20, BEP-20 etc.

Question is: is it an expected behaviour and apps should handle it on gui side for all platforms?

Related: https://github.com/KomodoPlatform/WebDEX/issues/815

cc: @ologunB

[ologunB]

I checked the day it was implemented for IRIS and IRIS_IBC, that other protocol tokens arent deactivated when parent is deactivated, and it was only for IRIS.

I check again checked now on mobile, and you can deactivate them separately. Only IRIS/IRIS_IBC behaves like that from the coins I tested with.

flutter: {"result":{"coin":"AVAX","cancelled_orders":[],"tokens":[]}}
flutter: {"result":{"coin":"1INCH-AVX20","cancelled_orders":[],"tokens":[]}}
flutter: {"result":{"coin":"ETH","cancelled_orders":[],"tokens":[]}}
flutter: {"result":{"coin":"1INCH-ERC20","cancelled_orders":[],"tokens":[]}}
flutter: {"result":{"coin":"BNB","cancelled_orders":[],"tokens":[]}}
flutter: {"result":{"coin":"ADX-BEP20","cancelled_orders":[],"tokens":[]}}

Also, you can still activate all protocols separately. Except SLPs, IRISs

[sergeyboyko0791]

@yurii-khi @ologunB thanks for the question!

We deactivate tokens if only they were activated via enable_eth_coin_with_tokens, enable_erc20, enable_bch_with_tokens, enable_slp, enable_tendermint_with_assets, enable_tendermint_token RPCs.

disable_coin RPC for platform coin (e.g. BNB) also automatically deactivates all its tokens (e.g. KMD-BEP20). Looks like it work like this for all platforms/tokens, including ERC-20, BEP-20 etc.

This is done because such tokens are completely dependent on the platform coins.

If the token is activated via the legacy enable (for ERC20 only), it works independently from other tokens and platform coins, so they won't be deactivated along with the platform (ETH) coin.

[yurii-khi]

If the token is activated via the legacy enable (for ERC20 only), it works independently from other tokens and platform coins, so they won't be deactivated along with the platform (ETH) coin.

Ok, got it now, thanks @sergeyboyko0791 for clarification. We're not using legacy enable rpc anymore, thats probably the reason.
So, if we activate bep-20 tokens with enable_erc20 - they will be automatically deactivated when disabling BNB, and that's intentional, am I correct?

[sergeyboyko0791]

Yes, right

So, if we activate bep-20 tokens with enable_erc20 - they will be automatically deactivated when disabling BNB, and that's intentional, am I correct?

[ca333]

@yurii-khi @ologunB thanks for the question!

We deactivate tokens if only they were activated via enable_eth_coin_with_tokens, enable_erc20, enable_bch_with_tokens, enable_slp, enable_tendermint_with_assets, enable_tendermint_token RPCs.

disable_coin RPC for platform coin (e.g. BNB) also automatically deactivates all its tokens (e.g. KMD-BEP20). Looks like it work like this for all platforms/tokens, including ERC-20, BEP-20 etc.

This is done because such tokens are completely dependent on the platform coins.

If the token is activated via the legacy enable (for ERC20 only), it works independently from other tokens and platform coins, so they won't be deactivated along with the platform (ETH) coin.

This is considered a global bug - please fix.

Expected behaviour:

If i try to disable a "parental chain" (EMV, tendermint, etc) mm2 should either disable only the coin-specific interface / functionalities however not fully remove it from the mm2 runtime scope since otherwise tokens running on these "parental chains" just get killed although this is not intended by the user. This means, handle coin/token activation states independently of the "parental-chain" dependencies to prevent such situations. In our case and in regards to the resource-limitation I think it should be sufficient to execute a small refactoring towards the below pseudo-code logic - so we technically fake what I requested above and just gimmick a "coin-deactivation" but in reality we keep the parental chain running in case there is a active "child token" in the runtime scope - otherwise we can kill it.

if(mm2.call == disable && mm2.parental_chain.protocol_type == "parental_chain" && active_coins.protocols.includes(parental_chain.protocol))
    throw exception; // do nothing & keep parental_chain running
else disable(parental_chain.name);

Note: disable refers to any coin/token deactivation method / same applies to assumptions ref. enable

Obviously for future iterations and as resources free up it d make sense to further polish this layer. e.g. where mm2 "just knows when to kill a parental chain, when to launch it, also maybe in idle mode to spin up a few syncing threads to update pub-key related data thats not even in the "active_coins" scope, et cetera" and basically doing everything in the background and without the user/GUI asking for it. I am sure you know what I mean. But yea, thats the god-level polishing phase for after the v1 release, so we have exciting times ahead for the coming year or two :)

NB: Please feel free to propose a diff. refactoring / fix - the above should just be taken as inspiration/hints and independent feedback ref. dev friendly interface expectations. But its a bug and needs take care of so GUI-devs/cli-users don't end up having to do this kind of sanity checks.

[artemii235]

NB: Please feel free to propose a diff. refactoring / fix - the above should just be taken as inspiration/hints and independent feedback ref. dev friendly interface expectations. But its a bug and needs take care of so GUI-devs/cli-users don't end up having to do this kind of sanity checks.

As mentioned in the corresponding WebDEX issue, we can return an error if platform coin has active dependent tokens (with a list of them). This way, the API will do all required sanity checks and provide a clear feedback on what's wrong.

[sergeyboyko0791]

This makes sense! I'd also add the disable_tokens: bool and cancel_orders: bool parameters to the request. If disable_tokens == true and there are tokens that depend on the platform coin, return an error. Same behaviour with cancel_orders.

As mentioned in the corresponding WebDEX issue, we can return an error if platform coin has active dependent tokens (with a list of them). This way, the API will do all required sanity checks and provide a clear feedback on what's wrong.

[sergeyboyko0791]

I prepared a fix. Now we return an error if there are dependent tokens and disable_tokens is not present or equals to false.
The fix is small, so I decided to make it even if we don't need such disable_tokens parameter.

[yurii-khi]

I prepared a fix. Now we return an error if there are dependent tokens and disable_tokens is not present or equals to false. The fix is small, so I decided to make it even if we don't need such disable_tokens parameter.

Does it fix inconsistent behaviour of disable_coin rpc, described in #1648? It looks like disable_coin still behaves differently depending on previous actions (activation method). Please correct me if I'm wrong.

I believe, it would be much better if we allow disabling parent coin without disabling children, like suggested in #1641 (comment)

[onur-ozkan]

The most practical way to avoid destroying child contexts of platform coins may be to implement a soft kill/delete algorithm. This would allow us to passive the related coin rather than completely dropping it from the memory. For example, if we have three tokens and one platform coin enabled, disabling the platform coin would only make it invisible to the outside, but it would remain in memory for future use cases involving token operations. The coin would only be dropped from memory once all of its tokens have been disabled.

There are two ways to implement this:

We could create a new RPC called "soft_disable" so that the GUI can prompt the user to either turn off all tokens of the platform coin or simply disable the coin itself while keeping the tokens.

Alternatively, we could handle this behind the scenes without exposing a new RPC.

Let me know your opinions so I can start the implementation.

[shamardy]

Alternatively, we could handle this behind the scenes without exposing a new RPC.

I like this option more, I don't think we should add a new RPC.

[onur-ozkan]

cc @ca333