Skip to content

Leave hot signer unlocked after adding account#1339

Merged
Jamchello merged 5 commits intodevelopfrom
unlock-hotwallets-after-adding
Jan 17, 2023
Merged

Leave hot signer unlocked after adding account#1339
Jamchello merged 5 commits intodevelopfrom
unlock-hotwallets-after-adding

Conversation

@Jamchello
Copy link
Copy Markdown
Contributor

No description provided.

mholtzman
mholtzman previously approved these changes Jan 13, 2023
@mholtzman mholtzman dismissed their stale review January 13, 2023 13:25

tests are failing

@goosewobbler goosewobbler added enhancement New feature or request UX hot signers labels Jan 13, 2023
@goosewobbler goosewobbler changed the title leave unlocked when adding account Leave hot signer unlocked after adding account Jan 14, 2023
done(e)
}
}, 2000)
}, 10_000)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

afterAll(() => {
clean()

if (signer.status !== 'locked') {
Copy link
Copy Markdown
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
Copy Markdown
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?

} catch (e) {
done(e)
}
}, 7_500)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there a specific change we made that is causing these tests to take so long?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is because the unlock is now taking place in the same function call as the creation as far as I can tell

@Jamchello Jamchello merged commit 73a4226 into develop Jan 17, 2023
wakamex pushed a commit to wakamex/framed that referenced this pull request Mar 2, 2026
* leave unlocked when adding account

* address PR comments

* reduce timeouts again

* correct order of tests, increase timeout for creating the account, close the signer afterAll if not done yet

* reduce tests to 7500ms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request hot signers UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants