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

Implement Wallet #469

Merged
merged 18 commits into from
Jun 10, 2020
Merged

Implement Wallet #469

merged 18 commits into from
Jun 10, 2020

Conversation

flodesi
Copy link
Contributor

@flodesi flodesi commented Jun 1, 2020

Summary of changes
Changes introduced in this pull request:

  • implement wallet for our node to manage keys
  • some private key signing functions that are required for the implementation of the wallet

Reference issue to close (if applicable)

Closes #464

@flodesi flodesi marked this pull request as ready for review June 3, 2020 14:48
austinabell
austinabell suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Can you please write some tests for all of this? The interface is a lot easier to verify this way and there is some weird specific logic that if keeping would be best to be tested

key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/errors.rs Outdated Show resolved Hide resolved
key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
@flodesi flodesi requested a review from austinabell June 5, 2020 01:06
ec2
ec2 reviewed Jun 8, 2020
View reviewed changes
key_management/src/keystore.rs
}

#[derive(Default, Clone, PartialEq, Debug, Eq)]
pub struct MemKeyStore {
Copy link
Member

Choose a reason for hiding this comment

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

Won't keys need to be persisted? Is there a plan for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just in memory storage, when the node stops and starts again, these keys are lost. There should be a way to persist the keys to be reused in some way

austinabell
austinabell reviewed Jun 9, 2020
View reviewed changes
key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Outdated Show resolved Hide resolved
key_management/src/keystore.rs Outdated Show resolved Hide resolved
key_management/src/wallet.rs Show resolved Hide resolved
@flodesi flodesi requested review from austinabell and ec2 June 9, 2020 21:27
ec2
ec2 approved these changes Jun 10, 2020
View reviewed changes
Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

Saving the keys to disk doesn't look implemented yet. I'm ok with opening up another issue for that. Everything else looks good though.

austinabell
austinabell approved these changes Jun 10, 2020
View reviewed changes
key_management/src/keystore.rs Outdated Show resolved Hide resolved
flodesi and others added 2 commits June 10, 2020 09:33
@flodesi flodesi merged commit 8d2a493 into master Jun 10, 2020
@flodesi flodesi deleted the jaden/wallet branch June 10, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ec2 ec2 ec2 approved these changes

@austinabell austinabell austinabell approved these changes

@ansermino ansermino Awaiting requested review from ansermino

@dutterbutter dutterbutter Awaiting requested review from dutterbutter

@StaticallyTypedAnxiety StaticallyTypedAnxiety Awaiting requested review from StaticallyTypedAnxiety

@RajarupanSampanthan RajarupanSampanthan Awaiting requested review from RajarupanSampanthan

@timvermeulen timvermeulen Awaiting requested review from timvermeulen

Assignees
No one assigned
Labels
Crypto Node Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Wallet for key management
3 participants
@flodesi @ec2 @austinabell