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

Multiple-URI support in parse_uri and make_uri #9756

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

U65535F
Copy link

@U65535F U65535F commented Jan 30, 2025

This pull request addresses the issues raised in #7737 and introduces a new feature to the Monero codebase: multi-URL support for parse_uri and make_uri functions.

Key Changes:

  • Fixed loop index misuse (replaced i with j for proper iteration).
  • Removed the usage of NA as a filler in certain cases.
  • General code cleanup for readability and maintainability.
  • Introduced new test cases in uri.cpp to validate multi-URL functionality.

Notes:
This PR was rewritten from the latest Monero repository rather than being forked from the original issue (#7737).
All builds are successfully passing, and existing tests have cleared without any regressions (except for one test, please check actions).

Please check for any overlooked issues or mistakes, especially in the multi-URL parsing logic.
Validate the new test cases and suggest improvements to ensure robustness.

@U65535F U65535F changed the title Multiple-URL Multiple-URI support in parse_uri and make_uri Jan 30, 2025
@U65535F
Copy link
Author

U65535F commented Jan 31, 2025

@selsta

@selsta
Copy link
Collaborator

selsta commented Jan 31, 2025

Thank you. The URI functional test case fails now. Can you take a look?

https://github.com/monero-project/monero/actions/runs/13057491252/job/36433416280?pr=9756#step:11:1471

@U65535F
Copy link
Author

U65535F commented Jan 31, 2025

Alright, will ping you when it's done. It seems like I didn't update the python source files to comply with the new updated C++ code.

@iamamyth
Copy link

As illustrated by the test failures, this PR would introduce a backwards-incompatible change to the wallet RPC interface, which generally should not be done.

@iamamyth
Copy link

Two possible ways to address the compatibility issue:

  1. Introduce a new version of the endpoint which works with only the new format, retaining the old one (you could still transform requests to the old endpoint into the new format upon receipt and share the address construction code).
  2. Modify the endpoint to detect "old format" and "new format" requests, and pick the appropriate logic (e.g. payments field present implies new format, or address field present implies old format).

@U65535F U65535F marked this pull request as draft February 1, 2025 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants