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

Making wallet integration tests use cryptographic signatures #435

Conversation

@willmeister
Copy link
Collaborator

commented Sep 9, 2019

Description

Adds cryptographic signatures and signature checking for wallet and aggregator.

Metadata

Fixes

Contributing Agreement

willmeister added 2 commits Sep 9, 2019
Copy link
Contributor

left a comment

This is an extremely solid PR! I caught just one slight mistake (forgot to remove a reference to AGGREGATOR_ADDRESS) but other than that it looks good! It's a little light on docstrings, but those in glass houses shouldn't throw stones as I've deff sacrificed thorough commenting recently with all that needs to be done.

listAccounts(): Promise<string[]>
createAccount(password: string): Promise<string>
unlockAccount(address: string, password: string): Promise<void>
lockAccount(address: string): Promise<void>
sign(address: string, message: string): Promise<string>

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

Great--glad you pulled this out

* @returns The signer's address.
*/
verifyMessage(message: string, signature: string): string
}

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

Looks great!

@@ -17,6 +22,7 @@ import {
PIGI_TOKEN_TYPE,
TokenType,
State,
AGGREGATOR_ADDRESS,

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

Ah looks like you forgot to remove the import

@@ -44,13 +49,17 @@ describe('MockAggregator', async () => {

describe('applyTransaction', async () => {
it('should update bobs balance using applyTransaction to send 5 tokens', async () => {
const transaction = {
tokenType: UNI_TOKEN_TYPE,
recipient: 'bob',

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

Ah an interesting note is we currently allow people to send to "invalid" addresses like this. I made an issue about this so we don't forget to add a check in the state machine which can filter out these sorts of addresses #436

public verifyMessage(message: string, signature: string): string {
return signature
}
}

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

This is super useful!

@@ -23,28 +17,42 @@ describe('Mock Client/Aggregator Integration', async () => {
let accountAddress
let aggregator
let unipigWallet
const walletPassword = 'Really great password'

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 10, 2019

Contributor

😁

willmeister added 6 commits Sep 11, 2019
@willmeister willmeister merged commit 303083f into plasma-group:master Sep 11, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willmeister willmeister deleted the willmeister:feat/417/AddSigningForKeyManagement branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.