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] Implement methods used in recover_funds_of_swap for Tendermint. #1527

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

artemii235
Copy link
Member

@artemii235 artemii235 commented Nov 2, 2022

Also implemented validate_address.
Add dummy error return for others to avoid occasional panics.

#1432

This is one more P0 required for the upcoming WebDEX release.

Also implemented validate_address.
Add dummy error return for others to avoid occasional panics.
@artemii235 artemii235 changed the title [wip] Implement methods used in recover_funds_of_swap for Tendermint. [r2r] Implement methods used in recover_funds_of_swap for Tendermint. Nov 2, 2022
@artemii235 artemii235 added the P0 label Nov 2, 2022
shamardy
shamardy previously approved these changes Nov 2, 2022
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!

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Great work. Just couple non-blocker suggestions.

let request = GetTxsEventRequest {
events: vec![events_string],
pagination: None,
order_by: 0,
Copy link
Member

Choose a reason for hiding this comment

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

TendermintResultOrder::Ascending can be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ozkanonur I got expected i32, found enum cosmrs::tendermint_rpc::Order error when tried to use it. TendermintResultOrder is used in tx_search and block_search, but not for GetTxsEventRequest.

Copy link
Member

Choose a reason for hiding this comment

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

TendermintResultOrder::Ascending

I thught they implemented into for numbers. I will create new Order enum which implements TendermintResultOrder type and also can convert to i32. Since this PR is P0 I will do this refactoring on my side.

Comment on lines 1302 to 1324
match AccountId::from_str(address) {
Ok(account) => {
if account.prefix() == self.account_prefix {
ValidateAddressResult {
is_valid: true,
reason: None,
}
} else {
ValidateAddressResult {
is_valid: false,
reason: Some(format!(
"Expected {} account prefix, got {}",
self.account_prefix,
account.prefix()
)),
}
}
},
Err(e) => ValidateAddressResult {
is_valid: false,
reason: Some(e.to_string()),
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Less nested version:

match AccountId::from_str(address) {
    Ok(account) if account.prefix() != self.account_prefix => ValidateAddressResult {
        is_valid: false,
        reason: Some(format!(
            "Expected {} account prefix, got {}",
            self.account_prefix,
            account.prefix()
        )),
    },
    Ok(_) => ValidateAddressResult {
        is_valid: true,
        reason: None,
    },
    Err(e) => ValidateAddressResult {
        is_valid: false,
        reason: Some(e.to_string()),
    },
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@ozkanonur Done, thanks for the note!

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Great!

@artemii235 artemii235 merged commit 429ce2b into dev Nov 3, 2022
@artemii235 artemii235 deleted the tendermint-recover-funds-of-swap branch November 3, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants