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.

Sign up for GitHub

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

Jump to bottom

[r2r] Multi lightwalletd servers integration #1472

Merged
merged 20 commits into from
Oct 10, 2022
Merged

[r2r] Multi lightwalletd servers integration #1472

merged 20 commits into from
Oct 10, 2022

Conversation

laruh
Copy link
Member

@laruh laruh commented Sep 14, 2022 •
edited
Loading

issue #927
Firstly, there is an initialization of light clients using several lightwalletd servers urls.
Then program iterates them trying to send request.
The first successful result will be returned. In other case list of errors during the whole iteration will be returned.

artemii235
artemii235 requested changes Sep 28, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! First review iteration :)

mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
@laruh laruh changed the title [r2r] Multi lightwalletd servers integration [wip] Multi lightwalletd servers integration Sep 30, 2022
@laruh laruh changed the title [wip] Multi lightwalletd servers integration [r2r] Multi lightwalletd servers integration Oct 6, 2022
artemii235
artemii235 requested changes Oct 6, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Second review iteration.

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
@laruh
Copy link
Member Author

laruh commented Oct 7, 2022

@artemii235 if you dont mind, I decided to remove Mutex from blocks_db in this PR too, may be this task will be completed earlier than is_mine. Any way both implementations have the same commits from the dev branch.

@artemii235
Copy link
Member

if you dont mind, I decided to remove Mutex from blocks_db in this PR too, may be this task will be completed earlier than is_mine. Any way both implementations have the same commits from the dev branch.

@laruh Yes, that's ok :)

artemii235
artemii235 previously approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! @ozkanonur Could you take a look too, please? :)

onur-ozkan
onur-ozkan reviewed Oct 10, 2022
View reviewed changes
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! :) Only one note for future PR. LGTM 🔥

mm2src/coins/z_coin/z_rpc.rs
Fut: Future<Output = Result<tonic::Response<Res>, tonic::Status>>,
Fn: FnMut(&'a mut CompactTxStreamerClient<Channel>) -> Fut,
{
let mut errors = Vec::new();
Copy link
Member

Choose a reason for hiding this comment

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

Refer to #1480, this flow isn't good solution for handling multiple http connections. In scenario where we have 4-5 urls and first 3 always times out, the process will be unacceptably long.. There are similar implementation exists which will to be refactored as well.

Could you please add TODO note top of the this function including the issue url? We can refactor all of them in different PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

good note!
done

onur-ozkan
onur-ozkan approved these changes Oct 10, 2022
View reviewed changes
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.

🚀

@artemii235 artemii235 merged commit a32bba4 into dev Oct 10, 2022
@artemii235 artemii235 deleted the multi_lightwalletd_servers_integration branch October 10, 2022 11:22
@laruh laruh mentioned this pull request Feb 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onur-ozkan onur-ozkan onur-ozkan approved these changes

@artemii235 artemii235 artemii235 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laruh @artemii235 @onur-ozkan