simplewallet: handle transfers using a monero: URI #4208

moneromooo-monero · 2018-08-01T16:02:51Z

No description provided.

coneiric

One minor sticking point, and a couple clarifying questions. Other than that, changes look good to me.

coneiric · 2018-08-26T23:49:22Z

src/simplewallet/simplewallet.cpp

      {
        std::string extra_nonce;
        set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, payment_id8);
        r = add_extra_nonce_to_tx_extra(extra, extra_nonce);
+        local_args.pop_back();
+        payment_id_seen = true;
      }


Should r be updated here if parse_short_payment_id fails?

I don't think so, or it'd make the payment id mandatory.

coneiric · 2018-08-26T23:54:44Z

src/simplewallet/simplewallet.cpp

+        info.has_payment_id = true;
+      }
+      de.amount = amount;
+      ++i;


I'm a bit confused here: why is the index incremented by one here, or by two below in the else if block?

Because in that branch we found a parameter (a URI), and in the other one we found two (address and amount).

coneiric · 2018-08-26T23:56:10Z

src/simplewallet/simplewallet.cpp

+    else
+    {
+      fail_msg_writer() << tr("Invalid last argument: ") << local_args.back();
+      return true;


Typo? Why is true returned on error?

To mark the fact the command handler was run, that is what the caller expects.

coneiric · 2018-08-27T00:10:23Z

Also, something is making the cncrypto tests fail on buildbot. Is this a known bug?

moneromooo-monero · 2018-08-27T08:48:00Z

Yes

stoffu

I think the help message needs updating.

stoffu · 2018-08-27T13:12:48Z

src/simplewallet/simplewallet.cpp

@@ -4611,9 +4610,49 @@ bool simple_wallet::transfer_main(int transfer_type, const std::vector<std::stri
  size_t num_subaddresses = 0;
  for (size_t i = 0; i < local_args.size(); i += 2)


Now this i += 2 should be dropped.

stoffu · 2018-08-27T13:26:03Z

src/simplewallet/simplewallet.cpp

+      r = cryptonote::get_account_address_from_str_or_url(info, m_wallet->nettype(), address_uri, oa_prompter);
+      if (!payment_id_uri.empty())
+      {
+        if (!tools::wallet2::parse_short_payment_id(payment_id_uri, info.payment_id))


According to wallet2::parse_uri, both short and long payment ID formats seem to be supported. Why limit to short pID only?

Because an address cannot contain a long payment id.

Come to think of it, the long payment ID is being deprecated, so this way seems fine to me. I still think the long payment ID is allowed in a URI though, at least that's what I see in the unit test TEST(uri, long_payment_id).

Indeed it can. This was extracting from the address, where it can't. But I should check for a long here anyway, and not just at the end, you're right.

stoffu · 2018-08-27T15:00:11Z

src/simplewallet/simplewallet.cpp

+    }
+    else
+    {
+      fail_msg_writer() << tr("Invalid last argument: ") << local_args.back();


How about printing the URI parse error stored in error if not empty?

I guess. It's wallet2 though, so not translated, but it's better than nothing.

Ah, if there's an error, it might not be a URI, and I don't really want to start "guessing" to know whether showing the error is a good idea or not.

I still think it's useful to show an error message when the URI parsing fails, because it seems very easy to make incorrectly formatted URIs. How about showing it only when local_args.back() starts with "monero:"?

stoffu · 2018-08-27T15:02:26Z

src/simplewallet/simplewallet.cpp

@@ -2160,14 +2160,14 @@ simple_wallet::simple_wallet()
                           tr("Show the blockchain height."));
  m_cmd_binder.set_handler("transfer_original",
                           boost::bind(&simple_wallet::transfer, this, _1),
-                           tr("transfer_original [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] <address> <amount> [<payment_id>]"),
+                           tr("transfer_original [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] (URI | (<address> <amount>)) [<payment_id>]"),


I think it'd be accurate to also adjust this line as well:

Multiple payments can be made at once by adding <address_2> <amount_2> etcetera (before the payment ID, if it's included)"

stoffu · 2018-08-27T22:56:55Z

src/simplewallet/simplewallet.cpp

@@ -2161,14 +2161,14 @@ simple_wallet::simple_wallet()
  m_cmd_binder.set_handler("transfer_original",
                           boost::bind(&simple_wallet::transfer, this, _1),
                           tr("transfer_original [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] <address> <amount> [<payment_id>]"),


Hmm, was the change in this line removed by mistake?

stoffu · 2018-08-28T00:37:05Z

src/simplewallet/simplewallet.cpp

+      if (info.has_payment_id)
+        set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, info.payment_id);
+      else if (tools::wallet2::parse_payment_id(payment_id_uri, payment_id))
+        set_payment_id_to_tx_extra_nonce(extra_nonce, payment_id);


This line never gets executed due to the above logic:

if (!payment_id_uri.empty()) { if (!tools::wallet2::parse_short_payment_id(payment_id_uri, info.payment_id)) { fail_msg_writer() << tr("failed to parse short payment ID from URI"); return true; } ...

Confirmed:

[wallet 9xLMUj]: transfer monero:A1PbM6kDJs7eJnAjwwE7Bk3DH84iQbfKZF6x5i8KuBrhdZBhKgfwuTPCqTEjPZPKJMG32ioosAHR3KkwG97tDfZS5kSJJRN?tx_amount=1.2&tx_payment_id=1234123412341234123412341234123412341234123412341234123412341234 Wallet password: Error: failed to parse short payment ID from URI

stoffu · 2018-08-28T00:38:54Z

Please address my comment #4208 (comment)

moneromooo-monero · 2018-08-28T08:16:40Z

Yes, it appears I lost a few fixes in the last merge to the PR branch :( Fixing...

stoffu · 2018-08-28T13:18:24Z

src/simplewallet/simplewallet.cpp

+    }
+    else
+    {
+      fail_msg_writer() << tr("Invalid last argument: ") << local_args.back();


I still think it's useful to show an error message when the URI parsing fails, because it seems very easy to make incorrectly formatted URIs. How about showing it only when local_args.back() starts with "monero:"?

stoffu · 2018-08-28T13:42:16Z

src/simplewallet/simplewallet.cpp

-      set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, info.payment_id);
+      if (info.has_payment_id)
+        set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, info.payment_id);
+      else if (tools::wallet2::parse_payment_id(payment_id_uri, payment_id))


Could add the same warning:

message_writer() << tr("Unencrypted payment IDs are bad for privacy: ask the recipient to use subaddresses instead");

fluffypony

Reviewed

2c636e4 simplewallet: handle transfers using a monero: URI (moneromooo-monero)

coneiric reviewed Aug 27, 2018

View reviewed changes

stoffu reviewed Aug 27, 2018

View reviewed changes

moneromooo-monero force-pushed the truri branch 2 times, most recently from bd0a007 to 2710400 Compare August 27, 2018 14:56

stoffu reviewed Aug 27, 2018

View reviewed changes

moneromooo-monero force-pushed the truri branch from 2710400 to 433f5b0 Compare August 27, 2018 21:16

stoffu reviewed Aug 27, 2018

View reviewed changes

stoffu reviewed Aug 28, 2018

View reviewed changes

moneromooo-monero force-pushed the truri branch from 433f5b0 to a01809a Compare August 28, 2018 08:32

stoffu reviewed Aug 28, 2018

View reviewed changes

simplewallet: handle transfers using a monero: URI

2c636e4

moneromooo-monero force-pushed the truri branch from a01809a to 2c636e4 Compare August 28, 2018 19:56

fluffypony approved these changes Sep 14, 2018

View reviewed changes

fluffypony merged commit 2c636e4 into monero-project:master Sep 14, 2018

fluffypony added a commit that referenced this pull request Sep 14, 2018

Merge pull request #4208

d794abb

2c636e4 simplewallet: handle transfers using a monero: URI (moneromooo-monero)

stoffu mentioned this pull request Jun 4, 2020

Aeon upstream 7 aeonix/aeon#180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplewallet: handle transfers using a monero: URI #4208

simplewallet: handle transfers using a monero: URI #4208

moneromooo-monero commented Aug 1, 2018

coneiric left a comment

coneiric Aug 26, 2018

moneromooo-monero Aug 27, 2018

coneiric Aug 26, 2018

moneromooo-monero Aug 27, 2018

coneiric Aug 26, 2018

moneromooo-monero Aug 27, 2018

coneiric commented Aug 27, 2018

moneromooo-monero commented Aug 27, 2018

stoffu left a comment •

edited

Loading

stoffu Aug 27, 2018

stoffu Aug 27, 2018

moneromooo-monero Aug 27, 2018

stoffu Aug 27, 2018

moneromooo-monero Aug 27, 2018

stoffu Aug 27, 2018

moneromooo-monero Aug 27, 2018

moneromooo-monero Aug 27, 2018

stoffu Aug 28, 2018

stoffu Aug 27, 2018

stoffu Aug 27, 2018

stoffu Aug 28, 2018

stoffu commented Aug 28, 2018

moneromooo-monero commented Aug 28, 2018

stoffu Aug 28, 2018

stoffu Aug 28, 2018

fluffypony left a comment

		@@ -4611,9 +4610,49 @@ bool simple_wallet::transfer_main(int transfer_type, const std::vector<std::stri
		size_t num_subaddresses = 0;
		for (size_t i = 0; i < local_args.size(); i += 2)

simplewallet: handle transfers using a monero: URI #4208

simplewallet: handle transfers using a monero: URI #4208

Conversation

moneromooo-monero commented Aug 1, 2018

coneiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coneiric commented Aug 27, 2018

moneromooo-monero commented Aug 27, 2018

stoffu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu commented Aug 28, 2018

moneromooo-monero commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluffypony left a comment

Choose a reason for hiding this comment

stoffu left a comment •

edited

Loading