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

Leave hot signer unlocked after adding account #1339

Merged
merged 5 commits into from
Jan 17, 2023
Merged

Leave hot signer unlocked after adding account #1339

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c009fba
leave unlocked when adding account
Jamchello Jan 12, 2023
28b05c7
address PR comments
Jamchello Jan 13, 2023
5e1c4ae
reduce timeouts again
Jamchello Jan 13, 2023
1949a79
correct order of tests, increase timeout for creating the account, cl…
Jamchello Jan 16, 2023
f6cf927
reduce tests to 7500ms
Jamchello Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions main/signers/hot/RingSigner/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ class RingSigner extends HotSigner {
this.update()

// If signer was unlock -> update keys in worker
if (this.status === 'ok') this.unlock(password, cb)
else cb(null)
this.unlock(password, cb)
})
}

Expand All @@ -78,7 +77,7 @@ class RingSigner extends HotSigner {
this.update()

// If signer was unlock -> update keys in worker
if (this.status === 'ok') this.unlock(password, cb)
if (this.status === 'ok') this.lock(cb)
Jamchello marked this conversation as resolved.
Show resolved Hide resolved
else cb(null)
})
}
Expand Down
3 changes: 1 addition & 2 deletions main/signers/hot/SeedSigner/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ class SeedSigner extends HotSigner {
this.encryptedSeed = encryptedSeed
this.addresses = addresses
this.update()

cb(null)
this.unlock(password, cb)
})
}

Expand Down
36 changes: 19 additions & 17 deletions test/main/signers/hot/RingSigner/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ describe('Ring signer', () => {

afterAll(() => {
clean()

if (signer.status !== 'locked') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

after all the tests are run we should know what state the signer is in. what's this check for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in case there is an unfortunate failure in one of the test cases, I was thinking that this would precent the really long hanging build (looks like last one hung for ~6hrs?

signer.close()
}
log.transports.console.level = 'debug'
})

Expand Down Expand Up @@ -82,15 +84,15 @@ describe('Ring signer', () => {
signer = result

expect(err).toBe(null)
expect(signer.status).toBe('locked')
expect(signer.status).toBe('ok')
expect(signer.id).not.toBe(undefined)
expect(store(`main.signers.${signer.id}.id`)).toBe(signer.id)
done()
})
} catch (e) {
done(e)
}
}, 2000)
}, 10_000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mholtzman looks like the tests failed because this test timed out, which then lead to the signer not being assigned. I have set a longer timeout here as this prevents similar failures

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this happening locally or only in CI? I think this was an already existing problem in our Github actions flow so we should probably just fix the tests at some point to prevent these flaky failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, only in the CI


test('Scan for signers', (done) => {
jest.useFakeTimers()
Expand Down Expand Up @@ -132,7 +134,7 @@ describe('Ring signer', () => {
hot.createFromKeystore(signers, keystore, 'test', PASSWORD, (err, result) => {
signer = result
expect(err).toBe(null)
expect(signer.status).toBe('locked')
expect(signer.status).toBe('ok')
expect(signer.id).not.toBe(undefined)
done()
})
Expand Down Expand Up @@ -177,7 +179,7 @@ describe('Ring signer', () => {
} catch (e) {
done(e)
}
}, 1000)
}, 2000)

test('Add private key from keystore', (done) => {
try {
Expand All @@ -195,6 +197,18 @@ describe('Ring signer', () => {
}
}, 2000)

test('Lock', (done) => {
try {
signer.lock((err) => {
expect(err).toBe(null)
expect(signer.status).toBe('locked')
done()
})
} catch (e) {
done(e)
}
}, 2000)

test('Unlock with wrong password', (done) => {
try {
signer.unlock('Wrong password', (err) => {
Expand Down Expand Up @@ -277,18 +291,6 @@ describe('Ring signer', () => {
}
}, 500)

test('Lock', (done) => {
Jamchello marked this conversation as resolved.
Show resolved Hide resolved
try {
signer.lock((err) => {
expect(err).toBe(null)
expect(signer.status).toBe('locked')
done()
})
} catch (e) {
done(e)
}
}, 2000)

test('Sign message when locked', (done) => {
try {
signer.signMessage(0, 'test', (err) => {
Expand Down
30 changes: 16 additions & 14 deletions test/main/signers/hot/SeedSigner/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ describe('Seed signer', () => {

afterAll(() => {
clean()

if (signer.status !== 'locked') {
signer.close()
}
log.transports.console.level = 'debug'
})

Expand All @@ -65,14 +67,26 @@ describe('Seed signer', () => {
hot.createFromPhrase(signers, mnemonic, PASSWORD, (err, result) => {
signer = result
expect(err).toBe(null)
expect(signer.status).toBe('locked')
expect(signer.status).toBe('ok')
expect(signer.addresses.length).toBe(100)
expect(store(`main.signers.${signer.id}.id`)).toBe(signer.id)
done()
})
} catch (e) {
done(e)
}
}, 10_000)

test('Lock', (done) => {
try {
signer.lock((err) => {
expect(err).toBe(null)
expect(signer.status).toBe('locked')
done()
})
} catch (e) {
done(e)
}
}, 2000)

test('Scan for signers', (done) => {
Expand Down Expand Up @@ -177,18 +191,6 @@ describe('Seed signer', () => {
}
}, 500)

test('Lock', (done) => {
try {
signer.lock((err) => {
expect(err).toBe(null)
expect(signer.status).toBe('locked')
done()
})
} catch (e) {
done(e)
}
}, 2000)

test('Sign message when locked', (done) => {
try {
signer.signMessage(0, 'test', (err) => {
Expand Down