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

WalletApi: publicMultisigSignerKey method #3599

Merged
merged 1 commit into from Jun 8, 2018

Conversation

4 participants
@naughtyfox
Contributor

naughtyfox commented Apr 10, 2018

Multisig GUI wallet needs to know it's signer key

@moneromooo-monero

This should call get_multisig_signer_public_key (though it's unlikely to get out of sync now, but this changed in the past). Also it's dangerous when the wallet isn't multisig (see the function of the same name taking a key).

@naughtyfox naughtyfox force-pushed the naughtyfox:signer-key-api branch from b312466 to d010bec Apr 11, 2018

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 11, 2018

This should call get_multisig_signer_public_key

Done. Check for exception and return empty key string if it's not multisig wallet (the only possible reason for this function to throw)

Also it's dangerous when the wallet isn't multisig

Is checking wallet for being multisig enough to make it safe?

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 11, 2018

Is checking wallet for being multisig enough to make it safe?

Should be,though you don't get the ability to get the key before it gets multisig.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 11, 2018

Should be,though you don't get the ability to get the key before it gets multisig

That's right. Before wallet became multisig this method will return empty string. I don't consider this situation as an error so don't set error string, i think returning empty string is enough since it's not ambiguous.

@stoffu

stoffu approved these changes Apr 13, 2018

@@ -408,6 +408,12 @@ struct Wallet
*/
virtual std::string publicSpendKey() const = 0;
/*!
* \brief publicSignerKey - returns public signer key
* \return - public signer key for multisig wallet or spend public for classic wallet

This comment has been minimized.

@stoffu

stoffu Apr 13, 2018

Contributor

or spend public for classic wallet

Wait, this description doesn't agree with the implementation and should be dropped.

How about renaming the function to something like publicMultisigSignerKey() to emphasize that it's specific to multisig?

This comment has been minimized.

@naughtyfox

naughtyfox Apr 13, 2018

Contributor

no problem!

This comment has been minimized.

@naughtyfox

naughtyfox Apr 13, 2018

Contributor

Renamed method and updated descritption

@naughtyfox naughtyfox force-pushed the naughtyfox:signer-key-api branch from d010bec to 3f84665 Apr 13, 2018

@stoffu

stoffu approved these changes Apr 13, 2018

@naughtyfox naughtyfox force-pushed the naughtyfox:signer-key-api branch from 3f84665 to 8787fd8 Apr 13, 2018

@naughtyfox naughtyfox changed the title from WalletApi: publicSignerKey method to WalletApi: publicMultisigSignerKey method Apr 13, 2018

@moneromooo-monero

I'd prefer if it was testing for multisigness before calling the wallet2 API, as it'd avoid the exception, but I'll accept the patch either way.

@luigi1111 luigi1111 merged commit 8787fd8 into monero-project:master Jun 8, 2018

6 checks passed

buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win64 Build done.
Details

luigi1111 added a commit that referenced this pull request Jun 8, 2018

Merge pull request #3599
8787fd8 WalletApi: publicMultisigSignerKey method (naughtyfox)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment