Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[r2r] disable_coin should fail if there are tokens dependent on the platform #1651

Merged
merged 6 commits into from Feb 23, 2023

Conversation

sergeyboyko0791
Copy link

@sergeyboyko0791 sergeyboyko0791 commented Feb 10, 2023

The original PR - #1649

@shamardy shamardy added this to Todo in MM 2.0 via automation Feb 13, 2023
@shamardy shamardy moved this from Todo to PR review in MM 2.0 Feb 13, 2023
DeckerSU
DeckerSU previously approved these changes Feb 14, 2023
Copy link
Collaborator

@DeckerSU DeckerSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! Just have one suggestion :)
Also a question about calling cancel on init_standalone_coin, it seems that this also removes dependant tokens. Is this the required behaviour for v2 coin initialization?

mm2src/mm2_main/src/rpc/lp_commands/lp_commands_legacy.rs Outdated Show resolved Hide resolved
@sergeyboyko0791
Copy link
Author

init_standalone_coin is used to initialize only one coin (like UTXO, so we can consider it as a platform) without tokens.
But later enable_eth_with_tokens and other similar RPCs should be refactored significantly: #1650 (comment)

Also a question about calling cancel on init_standalone_coin, it seems that this also removes dependant tokens. Is this the required behaviour for v2 coin initialization?

@sergeyboyko0791 sergeyboyko0791 changed the title disable_coin should fail if there are tokens dependent on the platform [r2r] disable_coin should fail if there are tokens dependent on the platform Feb 15, 2023
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last question/suggestion that would require others input

mm2src/mm2_main/src/rpc/lp_commands/lp_commands_legacy.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sergeyboyko0791 sergeyboyko0791 mentioned this pull request Feb 19, 2023
8 tasks
Copy link

@smk762 smk762 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected response returned here

smk762@pig:~/mm2_777$ ./disable.sh BCH
{"error":"There are currently matching orders, active swaps, or tokens dependent on 'BCH'","orders":{"matching":[],"cancelled":[]},"active_swaps":[],"dependent_tokens":["ASLP-SLP"]}
smk762@pig:~/mm2_777$ ./disable.sh IRIS
{"error":"There are currently matching orders, active swaps, or tokens dependent on 'IRIS'","orders":{"matching":[],"cancelled":[]},"active_swaps":[],"dependent_tokens":["ATOM-IBC_IRIS"]}
smk762@pig:~/mm2_777$ ./disable.sh ETH
{"error":"There are currently matching orders, active swaps, or tokens dependent on 'ETH'","orders":{"matching":[],"cancelled":[]},"active_swaps":[],"dependent_tokens":["APE-ERC20","BCH-ERC20","BUSD-ERC20","MINDS-ERC20"]}
smk762@pig:~/mm2_777$ ./disable.sh MATIC
{"error":"There are currently matching orders, active swaps, or tokens dependent on 'MATIC'","orders":{"matching":[],"cancelled":[]},"active_swaps":[],"dependent_tokens":["AAVE-PLG20","PGX-PLG20"]}

/// Get enabled coins to disable.
pub async fn get_tokens_to_disable(&self, ticker: &str) -> HashSet<String> {
/// If `ticker` is a platform coin, returns tokens dependent on it.
pub async fn get_dependent_tokens(&self, ticker: &str) -> HashSet<String> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub async fn get_dependent_tokens(&self, ticker: &str) -> HashSet<String> {
pub async fn get_platform_coin_tokens(&self, ticker: &str) -> HashSet<String> {

Comment on lines 76 to 79
let dependent_tokens = coins_ctx.get_dependent_tokens(&ticker).await;
if !dependent_tokens.is_empty() {
return ERR!("There are tokens dependent on '{}': {:?}", ticker, dependent_tokens);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let dependent_tokens = coins_ctx.get_dependent_tokens(&ticker).await;
if !dependent_tokens.is_empty() {
return ERR!("There are tokens dependent on '{}': {:?}", ticker, dependent_tokens);
}
let platform_coin_tokens = coins_ctx.get_platform_coin_tokens(&ticker).await;
if !platform_coin_tokens.is_empty() {
return ERR!("There are tokens dependent on '{}': {:?}", ticker, platform_coin_tokens);
}

@ca333 ca333 merged commit 2f98ace into KomodoPlatform:dev Feb 23, 2023
MM 2.0 automation moved this from PR review to Done Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MM 2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants