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

Add SLP UTXOs validation using BCHD API. #1094

Merged
merged 16 commits into from Oct 8, 2021
Merged

Conversation

artemii235
Copy link
Member

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.

Looks good! I left a few comments. Please take a look at them :)

mm2src/coins/utxo/bch.rs Show resolved Hide resolved
fn my_public_key(&self) -> &Public { self.utxo_arc.key_pair.public() }
fn my_public_key(&self) -> &Public {
let public = self.utxo_arc.key_pair.public();
println!("{:?}", public);

Choose a reason for hiding this comment

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

I guess it's not used anymore :)

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I forgot to remove it after debugging 🙂 I will push the fix shortly.

mm2src/coins/utxo/bchd_grpc.rs Show resolved Hide resolved
mm2src/coins/utxo/bchd_grpc.rs Show resolved Hide resolved
mm2src/coins/utxo/bchd_grpc.rs Show resolved Hide resolved
impl From<GrpcWebMultiUrlReqErr> for CheckSlpTransactionErr {
fn from(err: GrpcWebMultiUrlReqErr) -> Self {
CheckSlpTransactionErr {
// url is already present in the GrpcWebMultiUrlReqErr

Choose a reason for hiding this comment

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

How about cloning the url? We may remove the CheckSlpTransactionErr::kind field someday and forget to fill CheckSlpTransactionErr::to_url.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that's actually good to have a bit of redundancy.

@@ -1,18 +1,23 @@
/// https://slp.dev/

Choose a reason for hiding this comment

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

It looks like a doc comment related to the import below :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add more info for clarity 🙂

mm2src/common/grpc_web.rs Show resolved Hide resolved
shamardy
shamardy previously approved these changes Oct 6, 2021
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.

Approved! Only one non-blocking comment in addition to @sergeyboyko0791 comments.

mm2src/coins/qrc20.rs Outdated Show resolved Hide resolved
@artemii235
Copy link
Member Author

@sergeyboyko0791 I've pushed commit with fixes. Could you please review the PR again? 🙂

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.

Thanks for the fixes! Approve it :)

@artemii235 artemii235 merged commit bbb798f into dev Oct 8, 2021
@artemii235 artemii235 deleted the mm2.1-slp-swap-bchd branch October 8, 2021 05:43
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.

None yet

4 participants