-
Notifications
You must be signed in to change notification settings - Fork 36
Login with signature string in addition to mnemonic #4117
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
Conversation
add5991 to
b33403b
Compare
b33403b to
132e07b
Compare
pronebird
left a comment
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.
Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpnd/src/service/vpn_service.rs line 1484 at r1 (raw file):
} fn parse_secret(secret: &LoginSecret) -> Result<Mnemonic, AccountCommandError> {
I have a few issues wit that:
- Reusing error from different crate?
- VpnService is already 1.5k long, can this be extracted to somewhere else?
pronebird
left a comment
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.
Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpnc/src/commands/account.rs line 80 at r1 (raw file):
let request = match mode { VpnAccountMode::Api => StoreAccountRequest::Vpn { secret }, VpnAccountMode::Decentralised => StoreAccountRequest::Decentralised { secret },
Can we even use priivy login with decentralized mode?
pronebird
left a comment
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.
@pronebird reviewed 9 of 19 files at r1.
Reviewable status: 9 of 19 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpn-lib-uniffi/src/account.rs line 201 at r1 (raw file):
} fn parse_secret(secret: &LoginSecret) -> Result<Mnemonic, VpnError> {
Copy and paste from vpn_service. Any way we can share this function?
pronebird
left a comment
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.
@pronebird reviewed 7 of 19 files at r1, all commit messages.
Reviewable status: 16 of 19 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
pronebird
left a comment
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.
@pronebird reviewed 3 of 19 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
neacsu
left a comment
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @pronebird, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpn-lib-uniffi/src/account.rs line 201 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Copy and paste from vpn_service. Any way we can share this function?
I was looking around and I don't see a common dependency where this would fit, nor do we have something like a util crate. What do you think, should we introduce such a crate?
nym-vpn-core/crates/nym-vpnc/src/commands/account.rs line 80 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Can we even use priivy login with decentralized mode?
Privy probably not, but generating the signing key from entropy bytes might be useful for decentralized mode too. The general functionality is not limited to Privy nor to the centralized VPN
neacsu
left a comment
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.
Reviewable status: 2 of 23 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @pronebird, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpnd/src/service/vpn_service.rs line 1484 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
I have a few issues wit that:
- Reusing error from different crate?
- VpnService is already 1.5k long, can this be extracted to somewhere else?
Done.
b762d63 to
58f596b
Compare
pronebird
left a comment
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.
@pronebird reviewed 5 of 30 files at r2.
Reviewable status: 5 of 30 files reviewed, 2 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpn-lib-uniffi/src/account.rs line 201 at r1 (raw file):
Previously, neacsu (Bogdan-Ștefan Neacşu) wrote…
I was looking around and I don't see a common dependency where this would fit, nor do we have something like a
utilcrate. What do you think, should we introduce such a crate?
I think that nym-vpn-lib is a good place
pronebird
left a comment
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.
Reviewable status: 5 of 30 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-privy/src/error.rs line 5 at r2 (raw file):
#[derive(thiserror::Error, Debug, Clone, PartialEq)] pub enum PrivyError {
We do have plenty of crates. Perhaps let's move that to nym-vpn-lib along with new nym-utils?
pronebird
left a comment
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.
Reviewable status: 5 of 30 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpnc/src/commands/account.rs line 80 at r1 (raw file):
Previously, neacsu (Bogdan-Ștefan Neacşu) wrote…
Privy probably not, but generating the signing key from entropy bytes might be useful for decentralized mode too. The general functionality is not limited to Privy nor to the centralized VPN
Maybe we should reduce LoginType + VpnAccountMode to single type, for example:
enum VpnAccount {
Api(Mnemonic),
Priivy(PrivateKeyHex),
Decentralized(Mnemonic)
}
pronebird
left a comment
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.
@pronebird reviewed 25 of 30 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
neacsu
left a comment
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @pronebird, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-privy/src/error.rs line 5 at r2 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
We do have plenty of crates. Perhaps let's move that to
nym-vpn-libalong with new nym-utils?
Done.
nym-vpn-core/crates/nym-vpn-lib-uniffi/src/account.rs line 201 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
I think that nym-vpn-lib is a good place
Done.
nym-vpn-core/crates/nym-vpnc/src/commands/account.rs line 80 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
Maybe we should reduce LoginType + VpnAccountMode to single type, for example:
enum VpnAccount { Api(Mnemonic), Priivy(PrivateKeyHex), Decentralized(Mnemonic) }
Done.
pronebird
left a comment
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.
@pronebird reviewed 16 of 24 files at r3.
Reviewable status: 25 of 33 files reviewed, 1 unresolved discussion (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, and @trojanfoe)
nym-vpn-core/crates/nym-vpnd/src/service/vpn_service.rs line 1288 at r3 (raw file):
let mnemonic = nym_vpn_lib::login::parse_account_request(&store_request) .map_err(|err| AccountCommandError::InvalidSecret(err.to_string()))?; if store_request.centralised() {
Nit: I'd add is_ prefix to centralised() but that's minor and subjective
pronebird
left a comment
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.
@pronebird reviewed 8 of 24 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @agentpietrucha, @doums, @mrkiwi, @rokas-ambrazevicius, and @trojanfoe)
pronebird
left a comment
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @agentpietrucha, @doums, @mrkiwi, @rokas-ambrazevicius, and @trojanfoe)
Ticket
JIRA-VPN-4378
Description
Allow UI to login using the hex signature string from Privy
Checklist:
Screenshots (optional, if UI related)
This change is