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

[RELEASE] multisig entries for wallet api #3489

Merged
merged 2 commits into from May 31, 2018

Conversation

Projects
None yet
4 participants
@naughtyfox
Copy link
Contributor

commented Mar 23, 2018

Hey guys! We are making multisig gui wallet and we'd like these changes to be in upcoming release.

Added abilities in gui wallet api to:

  • create multisig wallets
  • multisig key images exchange
  • create, sign, commit multisig transactions
  • Refactored the facility to set Wallet status to work in atomic way.

Tested transaction sending on ubuntu 17.10
API usage example: https://github.com/naughtyfox/monero-multisig-api-example/blob/master/main.cpp

@stoffu
Copy link
Contributor

left a comment

There are some changes done and undone in different commits; if possible, please try to squash them before the final merge.

break;
}


This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Spurious?

} catch (const tools::error::not_enough_unlocked_money& e) {
m_status = Status_Error;
setStatusError("");

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

setStatusError() is called a few lines below. Mistake?

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 23, 2018

Author Contributor

actually no. in previous version of code there was something like this:

m_status = Status_Error;
....
m_status = Status_Error;
m_errorString = "...";

because api lib wanted a caller to know error happened before setting actual error string. I followed the same way since m_status was atomic

} catch (const tools::error::not_enough_money& e) {
m_status = Status_Error;
setStatusError("");

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Same

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 23, 2018

Author Contributor

answered above

} catch (const tools::error::tx_not_possible& e) {
m_status = Status_Error;
setStatusError("");

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Same

}

if (!ready) {
throw runtime_error("Multisig wallet is not finalized yet");

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Seems spurious, since checkMultisigWalletReady() is called just above

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 23, 2018

Author Contributor

you absolutely right, thank you! i wrote this code first then refactored a bit and forgot about this lines. will fix it

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 26, 2018

Author Contributor

i deleted redundant checks but github shows it still for some reason

multisig_tx_set txs;
txs.m_ptx = ptx_vector;
wallet2::multisig_tx_set wallet2::make_multisig_tx_set(const std::vector<pending_tx>& ptx_vector) const {
multisig_tx_set txs;

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Please conform to the same indentation convention already used in each file, which in this case is two whitespaces.

{
crypto::public_key pkey = get_multisig_signing_public_key(msk);
for (auto &ptx: txs.m_ptx) for (auto &sig: ptx.multisig_sigs) sig.signing_keys.insert(pkey);
}

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

No whitespace changes

@@ -821,6 +821,8 @@ namespace tools
a & m_ring_history_saved;
}

multisig_tx_set make_multisig_tx_set(const std::vector<pending_tx>& ptx_vector) const;

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

Maybe move this line next to save_multisig_tx() as they're closely related?

}

tools::wallet2::multisig_tx_set txSet;
if (!m_wallet->load_multisig_tx(binary, txSet, std::function<bool(const tools::wallet2::multisig_tx_set&)>())) {

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

The last argument can simply be {}

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 23, 2018

Author Contributor

that's it! thx :)

/**
* @brief makeMultisig - switches wallet in multisig state. The one and only creation phase for N / N wallets
* @param info - vector of multisig infos from other participants obtained with getMulitisInfo call
* @param threshold - number of required signatures to make valid transaction. Must be equal to number of participants (N) or N - 1

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 23, 2018

Contributor

I'd rephrase "number of required signatures" to "number of required signers".

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 23, 2018

Author Contributor

good point, i'll do it

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

Please do not make changes to indentations if there's no functional change.

Refactored: work with wallet api statuses to make setting and getting…
… operations atomic along with error strings

WalletApi: added method statusWithErrorString to atomically retrieve error with error string

@naughtyfox naughtyfox changed the title [WIP] multisig entries for wallet api [RELEASE] multisig entries for wallet api Mar 28, 2018

@naughtyfox naughtyfox force-pushed the naughtyfox:multisig-wallet-api branch from 93b056e to c9bc037 Mar 28, 2018

@naughtyfox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

squashed into 3 commits
@stoffu please check this out

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

@naughtyfox
Are you intentionally refusing to remove those indentation changes and spurious line addition/deletion, which I'm strongly against?

@naughtyfox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

@stoffu Sorry, just got your message about minimizing changes wrong. Brought it back

@@ -4955,18 +4955,19 @@ bool wallet2::save_multisig_tx(const multisig_tx_set &txs, const std::string &fi
return epee::file_io_utils::save_string_to_file(filename, ciphertext);
}
//----------------------------------------------------------------------------------------------------
wallet2::multisig_tx_set wallet2::make_multisig_tx_set(const std::vector<pending_tx>& ptx_vector) const {

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 28, 2018

Contributor

Please revert this indentation change.

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Mar 28, 2018

Author Contributor

why do you want to revert it? this function is used in WalletImpl::createTransaction

@@ -821,8 +822,6 @@ namespace tools
a & m_ring_history_saved;
}

multisig_tx_set make_multisig_tx_set(const std::vector<pending_tx>& ptx_vector) const;

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 28, 2018

Contributor

Please squash this change to the previous commit where this function was introduced.

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

There should be just two commits, one for the status-related refactoring and the other for the multisig feature. The last two commits should be squashed into the second commit, and the indentation change should be undone.

@naughtyfox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

i thought i removed all of identations... did i miss something?

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

Don't you see the problem in the color-coded diff that GitHub offers?

Ah, you just need to squash the last three commits into one. That'll clear the issue.

@naughtyfox naughtyfox force-pushed the naughtyfox:multisig-wallet-api branch from 86b0f7f to 235adb5 Mar 28, 2018

WalletApi: getMultisigInfo entry for gui wallets
WalletApi: makeMultisig call introduced

WalletApi: finalizeMultisig call introduced

WalletApi: new calls exportMultisigImages and importMultisigImages

WalletApi: method to return multisig wallet creation state

WalletApi: create multisig transaction, sign multisig transaction, commit transaction and get multisig data are added

WalletApi: identation and style fixes

@naughtyfox naughtyfox force-pushed the naughtyfox:multisig-wallet-api branch from 235adb5 to 5a96056 Mar 28, 2018

@naughtyfox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

Squashed into 2 commits:

  • refactoring
  • feature commit
@stoffu

stoffu approved these changes Mar 28, 2018

Copy link
Contributor

left a comment

Looks good!
Sorry for being a little snarky about style changes, there's been quite a few similar cases lately, and I seem to be getting a little nervous about them :)

@naughtyfox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

No worries! It's my fault i didn't read the contribution doc, that's my first open source contribution and i've been a bit distracted. Thank you for your patience!

auto cipher = m_wallet.m_wallet->save_multisig_tx(m_pending_tx);
return epee::string_tools::buff_to_hex_nodelimer(cipher);
} catch (const std::exception& e) {
m_status = Status_Error;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero May 30, 2018

Contributor

Should this not lock the new mutex (and others below) ?

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox May 30, 2018

Author Contributor

It seems PendingTransactionImpl class is not thread safe and there's no mutex at all. I just followed the same logic. If you want i can fix it

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

No it's fine, I did not know it was a different m_status field.

@luigi1111 luigi1111 merged commit 5a96056 into monero-project:master May 31, 2018

5 checks passed

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 May 31, 2018

Merge pull request #3489
47fdb74 WalletApi: getMultisigInfo entry for gui wallets... (naughtyfox)
47fdb74 Refactored: work with wallet api statuses to make setting and getting operations atomic along with error strings (naughtyfox)

@stoffu stoffu referenced this pull request Jul 4, 2018

Merged

For 0.12.3.0 #4092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.