Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/remote key manager #202
base: feature/p2pmessaging
Are you sure you want to change the base?
Feature/remote key manager #202
Changes from all commits
e53d002
6ad4672
736638c
2d8519c
706be90
5275f85
7b829d3
6ac6520
f1d0e66
4444029
0fbe28b
7a8a5d0
838d4bb
8155a26
65fea20
0616570
f85b7e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change this in code. In code we should have a globally accessible server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we put it in config??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ideally do it in the same way we've done it for Blockstack user repository. It also requires such a default for Blockstack stuff.
Look at line 127 of crux-wallet-client.ts
this.cruxBlockstackInfrastructure = options.blockstackInfrastructure || CruxSpec.blockstack.infrastructure;
Defaults are defined in
CruxSpec
. just read directly from there. No need to do theoptions.blockstackInfrastructure ||
part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this? Remove if it was for your personal debugging only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it was for catching errors from setupCruxMessenger but now we can remove it(options are being used till here only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'catching error' is not a valid purpose. 'catching error to _________' is a valid purpose
Here you've suppressed it and logged it. Any particular cases/errors you're trying to catch here? If this function is too failure prone then it may block other functionalities of SDK. So then it may be a good idea to suppress error here like youve done
But in that case we should figure out why its failing and fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID should be coming as object here. If it isn't then it's a bug! Can you fix it?
SecureReceiveSocket will be the right place to do it. Please think about why it's the right place to do it, let me know if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah seems so. will check it again and fix it. And yes SecureRecieveSocket will be best as it will make sure that it's consistent throughout (CruxID)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ts ignore and fix errors. Let me know if you're getting confused with 'args' and how to make it generic for arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do an optimization here -
We know the Crux ID. And we're doing secure communication on the sole basis of the fact that we know the ID's Public Key. So we don't need to do makeRemoteMessageCall to getPubKey. You can get a CruxUser from CruxId and get Public Key from there. How to get CruxUser? We need UserRepository. SecureCruxNetwork already has a userRepository, Just that it's a private variable at the moment. You can make it public, and access it via CruxProtocolMessenger->SecureCruxNetwork->userRepository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong naming of parameter in all methods - look at other KeyManager implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry will take care of it