Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Align personal_unlockAccount behaviour when permanent unlock is disabled #10060

Merged
merged 7 commits into from
Jan 15, 2019

Conversation

jam10o-new
Copy link
Contributor

If permanent unlocking is disabled (the default) then the duration argument will be ignored, and the account will be unlocked for a single signing.

Current behaviour throws an error.

> If permanent unlocking is disabled (the default) then the duration argument will be ignored, and the account will be unlocked for a single signing.

Current behaviour throws an error that is no longer relevant.
@parity-cla-bot
Copy link

It looks like @joshua-mir signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@tomusdrw
Copy link
Collaborator

Flashbacks: #6777
IMHO it's a bit misleading to ignore the parameter and it's better to explicitly error, but I'm open for arguments why not.

@jam10o-new
Copy link
Contributor Author

I can change this so it throws a different error, we no longer have geth compatibility mode so the error wasn't helpful. In addition I feel that the documentation is clear enough that we are ignoring duration.

@tomusdrw
Copy link
Collaborator

I'd prefer an error, cause in the past we had issues with some implicit, but unexpected behaviour (even if documented).

@jam10o-new jam10o-new added the A0-pleasereview 🤓 Pull request needs code review. label Dec 13, 2018
@Tbaut Tbaut added the M6-rpcapi 📣 RPC API. label Dec 13, 2018
@Tbaut Tbaut added this to the 2.3 milestone Dec 13, 2018
"Time-unlocking is only supported in --geth compatibility mode.",
Some("Restart your client with --geth flag or use personal_sendTransaction instead."),
"Time-unlocking is not supported when permanent unlock is disabled.",
Some("Use personal_sendTransaction or enable permanent unlocking, instead."),
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing whitespaces

let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"Time-unlocking is not supported when permanent unlock is disabled.","data":"Use personal_sendTransaction or enable permanent unlocking, instead."},"id":1}"#;
assert_eq!(tester.io.handle_request_sync(&request), Some(response.into()));
assert!(tester.accounts.sign(address, None, Default::default()).is_err(), "Should not unlock account.");
Copy link
Contributor

Choose a reason for hiding this comment

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

please configure your editor to handle whitespaces properly

@jam10o-new
Copy link
Contributor Author

(I was working with the documentation script, which is why github claims that the previous commit was made by devops, oops, please ignore.)

@5chdn 5chdn modified the milestones: 2.3, 2.4 Jan 10, 2019
@sorpaas sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 11, 2019
@tomusdrw tomusdrw merged commit 0edf8e3 into master Jan 15, 2019
@tomusdrw tomusdrw deleted the jam-unlockaccount-fix branch January 15, 2019 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants