wallet: add multiple-URI support to `parse_uri` and `make_uri` #9830

U65535F · 2025-03-09T14:22:10Z

A rewritten version of #9756 that is almost similar to it.
The previous source got deleted somehow (apologies).

src/simplewallet/simplewallet.cpp

Tzadiko · 2025-03-18T16:31:53Z

src/simplewallet/simplewallet.cpp

+        de.is_subaddress = info.is_subaddress;
+        de.is_integrated = info.has_payment_id;
+
+        if (info.has_payment_id || !payment_id_uri.empty())


I know this isn't part of your change, but I'm curious why these two can contradict. In other words, is there a case where has_payment_id is false and payment_id_uri is not empty?

I don't think my comment here should hold up the PR, since it's not a change you made here, but I'd like to hear from someone else about this too.

src/wallet/api/wallet.h

src/wallet/wallet2.cpp

src/wallet/wallet_rpc_server.cpp

src/wallet/wallet_rpc_server_commands_defs.h

tests/functional_tests/uri.py

U65535F · 2025-03-20T08:16:23Z

@Tzadiko

Tzadiko · 2025-03-20T14:31:48Z

@Tzadiko

One tag is fine to prod me in case I missed the emails sent by Github. No need to tag me on each of my comments 👍

Tzadiko · 2025-03-20T15:28:18Z

You resolved a bunch of comments without them being addressed. I suggest we either come to a conclusion before they're resolved, or you make a change and push a commit. For example, after discussing I agree no further change is needed here, so it can be resolved.

src/wallet/wallet2.cpp

tests/functional_tests/uri.py

U65535F · 2025-03-20T17:10:06Z

You resolved a bunch of comments without them being addressed. I suggest we either come to a conclusion before they're resolved, or you make a change and push a commit. For example, after discussing I agree no further change is needed here, so it can be resolved.

Don't worry. Instead of letting you know every time (that seems spammy), I combined all of the suggestions into one commit and pushed them. Only time I will comment is when there is an issue.

U65535F · 2025-03-21T01:33:36Z

I wonder why my test fixture isn't getting executed properly.

Traceback (most recent call last):
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 369, in <module>
    URITest().run_test()
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 48, in run_test
    self.test_multi_uri_two_payments()
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 258, in test_multi_uri_two_payments
    wallet = multi_uri_common_variables["wallet"]
TypeError: 'URITest' object is not subscriptable
[TEST FAILED] uri

Tzadiko · 2025-03-21T01:47:29Z

I wonder why my test fixture isn't getting executed properly.

Traceback (most recent call last):
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 369, in <module>
    URITest().run_test()
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 48, in run_test
    self.test_multi_uri_two_payments()
  File "/__w/monero/monero/tests/functional_tests/uri.py", line 258, in test_multi_uri_two_payments
    wallet = multi_uri_common_variables["wallet"]
TypeError: 'URITest' object is not subscriptable
[TEST FAILED] uri

Call the fixture to get the returned dictionary, then access it: multi_uri_common_variables()["wallet"]

Tzadiko · 2025-03-21T13:31:19Z

Hey, Im going to stop reviewing this. There's been a persistent theme of you tagging me 8 hours after having not heard from me, and it's getting on my nerves. I've seen you do this with other people as well, people who have also spoken to you about this and asked you not to do it. I already spoke with you about this, and you said you're not going to continue pinging me. For example, not even 8 hours after leaving my last comment, you've pinged me again - I'm a human being, not a GPT, I need to sleep, I just woke up and I don't work for you (and even if I did, this would still be unacceptable). Not to mention, you're pinging me for issues that can be solved with simple debugging skills on code that you've supposedly written yourself - I'm unsure why you to ping me with regards to setting up a Python test fixture, there's plenty of documentation and examples online.

Tzadiko · 2025-03-21T14:58:18Z

I spent a bunch of time reviewing this and trying to help you. I wasn't going to leave another comment, until I saw your recent force-push. Instead of doing your part by doing some basic debugging to address a roadblock you encountered that was prompted by my comment, you reverted your code to a point beforewhich my comment existed for expedience. It's apparent that you don't care for improving the quality of your submission (which would in turn increase its chances of being accepted), if it requires just a tiny bit of effort for you to figure out how to write a test fixture in Python.

I'm just as new as you, but this sort of lazy development doesn't have a place in this codebase, in my opinion. I do not recommend anyone else review this PR, you'll be wasting your time. OP will ignore it at the earliest sign of inconvenience if it means he gets his code crammed through faster.

U65535F · 2025-03-21T15:19:34Z

I spent a bunch of time reviewing this and trying to help you. I wasn't going to leave another comment, until I saw your recent force-push. Instead of doing your part by doing some basic debugging to address a roadblock you encountered that was prompted by my comment, you reverted your code to a point beforewhich my comment existed for expedience. It's apparent that you don't care for improving the quality of your submission (which would in turn increase its chances of being accepted), if it requires just a tiny bit of effort for you to figure out how to write a test fixture in Python.

I'm just as new as you, but this sort of lazy development doesn't have a place in this codebase, in my opinion. I do not recommend anyone else review this PR, you'll be wasting your time. OP will ignore it at the earliest sign of inconvenience if it means he gets his code crammed through faster.

I realize now that my handling of this PR wasn’t ideal and clear enough. First off, a couple of clarifications:

When I force-pushed, I wasn’t discarding your review suggestions instead I squashed my commits into one to keep the history clean. The changes you pointed out are still included,
Mistakenly, I thought GitHub only notified people when they are directly tagged, so I didn’t realize my activity was pinging everyone too frequently. I'll be more mindful about that and be quieter.
Regarding the test fixture: it's apparent that you're right, I should have spent more time debugging on my own before asking. That would've saved reviewers some time.

I'm still fairly new to contributing to larger repositories, so I'm learning as I go (this time, the hard way). I really appreciate the effort you put into reviewing though it wasn’t at all wasted. I'll take this as a lesson to be more patient, clear about my next steps, and of course be considerate of reviewers time.

nahuhh · 2025-03-25T13:19:37Z

You'll need to drop the merge commit and use rebase instead

tests/functional_tests/uri.py

U65535F · 2025-03-26T14:56:11Z

@selsta Please review the PR. The new version (v0.18.4.0) is almost released.

Major improvements from previous old review:

Introduce new RPC commands: make_uri_v2 and parse_uri_v2: these will handle both single and multi recipient transactions. The old/legacy versions (i.e., make_uri and parse_uri) will be kept to fully ensure the backwards compatibility. (as suggested by iamamyth).
Removed all noticeable unnecessary edits and also fixed minor nits (thanks to Tzadiko for pointing them out).
Tests are passing successfully. Old tests have not been modified in any way.
EDIT: The failing test is unrelated to the PR!

U65535F · 2025-03-29T16:12:28Z

@selsta Hello, can you please tell me when you'll review this. I will patiently wait till that date.

selsta · 2025-03-29T16:41:42Z

The three points you have written above all sound good and were necessary to get this merged.

In file src/wallet/wallet_rpc_server_commands_defs.h, please update WALLET_RPC_VERSION_MINOR to indicate API changes.

selsta · 2025-05-09T19:13:03Z

I tested it in monero-wallet-cli and it looks good to me. I don't have any remaining suggestions at the moment.

selsta

I'm not sure if someone else is going to review this but from my side I'll add an approval.

sa8ab · 2025-06-12T22:38:04Z

Is it possible to have multiple integrated addresses on one transaction?
Is it maximum 1 integrated address and rest sub address or primary addresses?

U65535F · 2025-06-13T01:55:21Z

Is it possible to have multiple integrated addresses on one transaction?
Is it maximum 1 integrated address and rest sub address or primary addresses?

No multiple integrated addresses iirc.

selsta

From what I can tell this is currently broken, doing

transfer 4JtkiNvoi3iSPHHvcfXPPyj66zHQ9h1X9hmLCjDZ6kbNUhtYwDhAMm79p9GZ1iPCUhDMEvKMkMJdgb7JURAkCZUuhxNZCGqxVNDAmjcimV 0.001

[wallet 49C5ha]: show_transfer 35c55d6cc0733dc211ff15668978a3237e6e6f5b1bd48ac0a72d658b48961aee
Outgoing transaction found
txid: <35c55d6cc0733dc211ff15668978a3237e6e6f5b1bd48ac0a72d658b48961aee>
Height: 3489288
Timestamp: 2025-08-30 18:02:15Z
Amount: 0.001000000
Payment ID: 0000000000000000
Change: 0.000006040000
Fee: 0.000030580000
Destinations: 4JtkiNvoi3iSPHHvcfXPPyj66zHQ9h1X9hmLCjDZ6kbNUhtYwDhAMm79p9GZ1iPCUhDMEvKMkMJdgb7JURAkCZUuhxNZCGqxVNDAmjcimV: 0.000001000000
Note:

does not add the payment ID correctly

jeffro256

I have some disagreements about the code structure, and the URI format itself.

jeffro256 · 2025-08-30T17:57:39Z

src/wallet/wallet2.h

+    std::string custom_convert_to_url_format(const std::string &uri) const;
+    std::string make_uri(const std::vector<std::string> &addresses, const std::vector<uint64_t> &amounts, const std::vector<std::string> &recipient_names, const std::string &tx_description, std::string &error) const;


These new methods, including parse_uri(), do not need to be member functions of wallet2. I would recommend making them free functions. If they aren't to be free functions, at the very least mark the member functions as static, please.

jeffro256 · 2025-08-30T18:01:44Z

src/wallet/wallet_rpc_server.cpp

+  {
+    if (!m_wallet) return not_open(er);
+    std::string error;
+    std::string uri = m_wallet->make_uri(req.addresses, req.amounts, req.recipient_names, req.tx_description, error);


This is why we shouldn't have make_uri()/parse_uri() be member functions of wallet2: you need to have initialized a wallet2 instance for no reason.

jeffro256 · 2025-08-30T18:02:00Z

src/wallet/wallet_rpc_server.cpp

+    std::vector<std::string> addresses;
+    std::vector<uint64_t> amounts;
+    std::vector<std::string> recipient_names;
+    if (!m_wallet->parse_uri(req.uri, addresses, amounts, recipient_names, res.uri.tx_description, res.unknown_parameters, error))


Same point here

jeffro256 · 2025-08-30T18:39:10Z

src/wallet/wallet2.cpp

  }

-  if (amount > 0)
+  std::string uri = "monero:" + addresses_str;


I don't like the format of <all addresses>?<all amounts>&<all messages>, etc. Take a look at Bitcoin Unlimited's (node for Bcash) multiple address URI scheme: https://github.com/BitcoinUnlimited/BUIP/blob/master/086.md. The address, amount, and labels are next to each other for each output, which makes for more readable URIs. More importantly however, this allows the field for each output to remain undefined if desired. See this example for an actual use case: https://github.com/BitcoinUnlimited/BUIP/blob/master/086.md#example-1-bar-transaction. Here you can see that the amount field for a tip output at a bar is left empty, which triggers the client code to prompt the user to enter a desired amount. Overall, this type of URI format is way more flexible and future-proof than cramming values into a list based on field type, assuming the same structure for every single output in a transaction.

FWIW this is the format we decided on. I don't remember why exactly.

https://github.com/monero-project/monero/blob/master/docs/URI_SCHEME.md

Is it too late to change it if no one has a working implementation yet AFAIK?

#7731 (comment)

Here is some background on when it was decided on. I'm not sure if anyone actually implemented this.

Seems like @trasherdk and @benevanoff wanted a similar approach to what I am bringing up here: packing info per-output, instead of packing per-value-type.

Seems like @trasherdk and @benevanoff wanted a similar approach to what I am bringing up here: packing info per-output, instead of packing per-value-type.

As far as I know, XMRChat is the first and only one to use this, and we don't have anything in prod yet. We're fine with whatever format you prefer. Just hope it can be done quickly as we'd like to implement this soon.

I think the design goal here was for the multi-payment scheme to match the original single-payment scheme in case of a single payment, i.e. it merely extends the existing scheme, rather than replacing it, making for a graceful upgrade (in particular, newly generated single-payment URIs work fine with existing consumers). That said, one could argue a "graceful" upgrade creates a false continuity, as multi-payments might parse OK with software that only supports single payments, making for user headaches. In any case, looking at the current tests, something has gone way off the rails, because the tests now include an output parameter, which they shouldn't.

If you want to change the scheme, such that it's not compatible with the old one, it should be a clean break (i.e. a new scheme name), otherwise you end up violating the core principle of URI namespacing.

I get what you're saying. The solution is to change the scheme name so the old clients don't accidentally parse the new multi recipient URI. I wonder what should be the new URI scheme name if that's what you mean? Additionally, should the new functions parse and generate old scheme URIs if there's it's a single recipient?

We can't decide on minor issues like the scheme name before resolving the major question of whether to split the scheme at all. So, it seems any work should be on hold for now. I see two decent options:

Extend the scheme to preserve the single-payment format and intentionally differ from it for multi-payment URIs in a manner that makes it highly unlikely multi-payment URIs introduce false compatibility.

Keep the current single-payment URI scheme as-is, and introduce a new, multi-payment URI scheme.

In either case, I think single-payment URIs should work as-is (i.e. no changes to the single-payment use case, anything that previously worked will still work); otherwise, a lot of software breaks for a minor upgrade.

Can we not modify the old functions to reject the URI as multi-recipient if it contains any output parameter? If this is possible, this would eliminate the problem of false compatibility if I'm not wrong?

src/wallet/uri.cpp

U65535F · 2025-09-08T18:45:03Z

@selsta Can you review the new code. I think I have fixed the problem in the wallet cli. You can test it and see if it works now (I've tested from my side). One thing I want to point out is that it only adds payment id when you explicitly provide a payment_id parameter in the uri. Otherwise it wouldn't add, you've seen this and requested the changes. Additionally, @jeffro256 can you please review the new code (in the uri.cpp)? Is that the format that you expected?
Is the uri.cpp and uri.hpp at the right place? Should I introduce wrappers around the new functions in the wallet code? What do you think?

selsta · 2025-09-10T10:49:54Z

I will re-review but need a bit of time

FiatDemise · 2025-09-10T18:00:40Z

Am I understanding correctly that this is the new format? Is there supposed to be a question mark at the beginning?

monero:?output=address1;amount1;&output=address2;amount2;

iamamyth · 2025-09-10T19:52:33Z

Am I understanding correctly that this is the new format? Is there supposed to be a question mark at the beginning?

monero:?output=address1;amount1;&output=address2;amount2;

Per my comment, this looks wrong, as the original scheme specification (https://github.com/monero-project/monero/blob/master/docs/URI_SCHEME.md) contains no mention of an output param, and the single-output case should match the old format.

selsta added pending review wallet labels Mar 12, 2025

U65535F force-pushed the master branch from e9a27e2 to 857b2fe Compare March 16, 2025 12:55

Tzadiko reviewed Mar 18, 2025

View reviewed changes

Tzadiko reviewed Mar 20, 2025

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

Tzadiko reviewed Mar 20, 2025

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

Tzadiko reviewed Mar 20, 2025

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

Tzadiko reviewed Mar 20, 2025

View reviewed changes

tests/functional_tests/uri.py Outdated Show resolved Hide resolved

U65535F requested a review from Tzadiko March 21, 2025 13:24

U65535F force-pushed the master branch from 3ce21ca to 31a1b48 Compare March 21, 2025 14:30

U65535F force-pushed the master branch from 836ab9a to dd12547 Compare March 21, 2025 19:36

U65535F force-pushed the master branch 2 times, most recently from dd12547 to de81583 Compare March 25, 2025 13:36

iamamyth reviewed Mar 25, 2025

View reviewed changes

tests/functional_tests/uri.py Outdated Show resolved Hide resolved

U65535F force-pushed the master branch from aedba91 to ff689a4 Compare March 26, 2025 13:10

U65535F force-pushed the master branch from ff689a4 to 8d222aa Compare March 26, 2025 15:27

U65535F force-pushed the master branch from 8d222aa to 374c349 Compare March 29, 2025 18:23

woodser mentioned this pull request May 17, 2025

Support multiple destinations in make_uri and parse_uri [$350] haveno-dex/haveno#83

Open

U65535F changed the title ~~Adding multiple-URI support in parse_uri and make_uri~~ wallet: add multiple-URI support to parse_uri and make_uri May 24, 2025

selsta approved these changes Jun 7, 2025

View reviewed changes

selsta suggested changes Aug 30, 2025

View reviewed changes

jeffro256 suggested changes Aug 30, 2025

View reviewed changes

U65535F force-pushed the master branch 7 times, most recently from 9d2f644 to 2b2ffe7 Compare September 5, 2025 18:39

BroccomaglosAdolfas reviewed Sep 5, 2025

View reviewed changes

src/wallet/uri.cpp Outdated Show resolved Hide resolved

U65535F force-pushed the master branch 8 times, most recently from 41c9746 to 4b10655 Compare September 7, 2025 15:39

add multi-URI support

2b43ecb

U65535F force-pushed the master branch from 4b10655 to 2b43ecb Compare September 8, 2025 18:37

		std::string custom_convert_to_url_format(const std::string &uri) const;
		std::string make_uri(const std::vector<std::string> &addresses, const std::vector<uint64_t> &amounts, const std::vector<std::string> &recipient_names, const std::string &tx_description, std::string &error) const;

Uh oh!

wallet: add multiple-URI support to parse_uri and make_uri #9830

Are you sure you want to change the base?

wallet: add multiple-URI support to parse_uri and make_uri #9830

Uh oh!

Conversation

U65535F commented Mar 9, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

U65535F commented Mar 20, 2025

Uh oh!

Tzadiko commented Mar 20, 2025

Uh oh!

Tzadiko commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

U65535F commented Mar 20, 2025

Uh oh!

U65535F commented Mar 21, 2025

Uh oh!

Tzadiko commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tzadiko commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tzadiko commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

U65535F commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nahuhh commented Mar 25, 2025

Uh oh!

Uh oh!

U65535F commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

U65535F commented Mar 29, 2025

Uh oh!

selsta commented Mar 29, 2025

Uh oh!

selsta commented May 9, 2025

Uh oh!

selsta left a comment

Choose a reason for hiding this comment

Uh oh!

sa8ab commented Jun 12, 2025

Uh oh!

U65535F commented Jun 13, 2025

Uh oh!

selsta left a comment

Choose a reason for hiding this comment

Uh oh!

jeffro256 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffro256 Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wallet: add multiple-URI support to `parse_uri` and `make_uri` #9830

wallet: add multiple-URI support to `parse_uri` and `make_uri` #9830

Tzadiko commented Mar 21, 2025 •

edited

Loading

Tzadiko commented Mar 21, 2025 •

edited

Loading

Tzadiko commented Mar 21, 2025 •

edited

Loading

U65535F commented Mar 21, 2025 •

edited

Loading

U65535F commented Mar 26, 2025 •

edited

Loading

jeffro256 Aug 30, 2025 •

edited

Loading

selsta Aug 30, 2025 •

edited

Loading

iamamyth Sep 10, 2025 •

edited

Loading

U65535F Sep 11, 2025 •

edited

Loading