-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Multiple-URI support in parse_uri
and make_uri
#9756
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
Conversation
Thank you. The |
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. |
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. |
Two possible ways to address the compatibility issue:
|
@selsta @iamamyth So after a long trial and error, I've managed to reach all the requirements. It passes all the tests and is backwards compatible (fully or partially based on how you view it). The new format is basically the old format but for the multiple values, they're separated by a delimitter which in this case is a
becomes
The rest of the data, that is:
remains the same. To overcome this, I could introduce a new legacy data type and slightly change the code to adjust it. Other than this, what do you think? |
I will take a closer look within the next days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main issue currently is the RPC incompatibility. What is your opinion on adding a separate RPC command for it?
src/wallet/wallet2.cpp
Outdated
@@ -6236,6 +6236,9 @@ std::string wallet2::make_background_keys_file_name(const std::string &wallet_fi | |||
//---------------------------------------------------------------------------------------------------- | |||
bool wallet2::parse_long_payment_id(const std::string& payment_id_str, crypto::hash& payment_id) | |||
{ | |||
if (payment_id_str.size() != 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new check seems redundant and unrelated to this PR. I would remove it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There is no requirement for this. I forgot to remove that.
KV_SERIALIZE(payment_id); | ||
END_KV_SERIALIZE_MAP() | ||
}; | ||
|
||
struct COMMAND_RPC_MAKE_URI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WALLET_RPC_VERSION_MINOR
has to be bumped. Generally I'm unsure about this change since it seems incompatible. Wouldn't it make more sense to add a separate COMMAND_RPC_MAKE_MULTI_URI
command instead of changing the existing one? I assume that would also reduce the diff in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It would reduce the difference. So the original tests will work as usual. But if there is a need for multi-URI, then the user has to call the separate RPC commands which accept multiple recipient parameters for URI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if there is a need for multi-URI, then the user has to call the separate RPC commands which accept multiple recipient parameters for URI.
This is preferable over breaking all existing clients in my opinion.
tests/functional_tests/uri.py
Outdated
|
||
if __name__ == '__main__': | ||
URITest().run_test() | ||
URITest().run_test() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a newline to all files where this symbol shows up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate. Which symbol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@selsta As I pointed in previous comments, introducing a new command would completely solve the RPC incompatibility. Thus, I also have to remove the overloads and convert them into a new function. What should I name the new commands? It would then need two new |
@selsta Any more suggestions except the new RPC commands? |
I'm still thinking about what's the best way here, @iamamyth what is your opinion? |
I think this whole implementation has gone in backwards order: There needs to be a clear URI scheme based on a decision regarding compatibility, which then dictates the implementation. I think there is a clear scheme, which aimed to be backwards-compatible by keeping the old fields and using a |
As far as the RPC design goes, I already addressed that question earlier: #9756 (comment) |
If I had to pick one of the options I suggested for the RPC change, it'd likely be (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)." Nearly all of the endpoint-specific code just marshals and unmarshals data, leaving it to the parse to do the heavy lifting. A new endpoint simplifies the work because the unmarshaling code can force required fields. As for the endpoint path, convention would just be to tack a version on the end, i.e. |
@selsta Obviously. Do you know how is it possible to solve this issue? If |
If introducing v2 endpoints, the old RPC should fail with a multi-recipient URI; that's a desirable result. For the wallet API, I'm not even sure it makes sense to preserve the old functions, but, if they're preserved, they should just wrap everything as a one-element vector and call-through to the new, vector-based code. |
Regarding wallet API compatibility/changes, please consult with the author of this PR: #9464. |
You don't really need |
@selsta @iamamyth I think I have addressed your compatibility issue properly. Introduced v2 versions of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not sure what exactly I should look at.
I haven't done a review, just skimmed over the changes and noticed some unnecessary edits, mainly in tests/functional_tests/uri.py
.
Other than that I left a comment regarding the API.
Maybe you can clarify how I can help?
src/wallet/api/wallet.h
Outdated
@@ -214,7 +214,9 @@ class WalletImpl : public Wallet | |||
virtual void startRefresh() override; | |||
virtual void pauseRefresh() override; | |||
virtual bool parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error) override; | |||
virtual bool parse_uri(const std::string &uri, std::vector<tools::wallet2::uri_data> &data, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two issues here:
- AFAICT we don't want to have "non-standard" types as arguments for methods in the Wallet API, so for simplicity I'd change
std::vector<tools::wallet2::uri_data> &data
to vectors ofstd::string
/std::uint64_t
here, see example below. - This belongs to the interface in
src/wallet/api/wallet2_api.h
and there it should look like this (with respect to the proposed change from point 1):
virtual bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error) = 0;
And here it should look like this (no virtual
in the beginning but override
at the end):
bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error) override;
Same goes for make_uri()
below.
@SNeedlewoods The reason I suggested pinging you was for your feedback on the wallet API changes, as you've put in some effort to establish a coherent interface. And, it seems your comments address those changes, as hoped. Thanks. |
Please note that I have no interest in formally reviewing/approving this PR, so, if/when it gains the necessary approvals, there's no sense in pinging me. |
@SNeedlewoods What exactly are those unnecessary edits? Could you please list them so I can remove. |
src/wallet/api/wallet.h
Outdated
@@ -214,7 +214,9 @@ class WalletImpl : public Wallet | |||
virtual void startRefresh() override; | |||
virtual void pauseRefresh() override; | |||
virtual bool parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error) override; | |||
virtual bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no virtual
keyword in the front here (it should only be used in the interface class src/wallet/api/wallet2_api.h
(here you can find some general information about "pure virtual function" and "interface class")), IMO it's unfortunate that many methods in src/wallet/api/wallet.h
already have this wrong, but for example these methods are considered to do it the "right way":
monero/src/wallet/api/wallet.h
Lines 83 to 138 in 84df774
Device getDeviceType() const override; | |
bool close(bool store = true); | |
std::string seed(const std::string& seed_offset = "") const override; | |
std::string getSeedLanguage() const override; | |
void setSeedLanguage(const std::string &arg) override; | |
// void setListener(Listener *) {} | |
int status() const override; | |
std::string errorString() const override; | |
void statusWithErrorString(int& status, std::string& errorString) const override; | |
bool setPassword(const std::string &password) override; | |
const std::string& getPassword() const override; | |
bool setDevicePin(const std::string &password) override; | |
bool setDevicePassphrase(const std::string &password) override; | |
std::string address(uint32_t accountIndex = 0, uint32_t addressIndex = 0) const override; | |
std::string integratedAddress(const std::string &payment_id) const override; | |
std::string secretViewKey() const override; | |
std::string publicViewKey() const override; | |
std::string secretSpendKey() const override; | |
std::string publicSpendKey() const override; | |
std::string publicMultisigSignerKey() const override; | |
std::string path() const override; | |
void stop() override; | |
bool store(const std::string &path) override; | |
std::string filename() const override; | |
std::string keysFilename() const override; | |
bool init(const std::string &daemon_address, uint64_t upper_transaction_size_limit = 0, const std::string &daemon_username = "", const std::string &daemon_password = "", bool use_ssl = false, bool lightWallet = false, const std::string &proxy_address = "") override; | |
bool connectToDaemon() override; | |
ConnectionStatus connected() const override; | |
void setTrustedDaemon(bool arg) override; | |
bool trustedDaemon() const override; | |
bool setProxy(const std::string &address) override; | |
uint64_t balance(uint32_t accountIndex = 0) const override; | |
uint64_t unlockedBalance(uint32_t accountIndex = 0) const override; | |
uint64_t blockChainHeight() const override; | |
uint64_t approximateBlockChainHeight() const override; | |
uint64_t estimateBlockChainHeight() const override; | |
uint64_t daemonBlockChainHeight() const override; | |
uint64_t daemonBlockChainTargetHeight() const override; | |
bool synchronized() const override; | |
bool refresh() override; | |
void refreshAsync() override; | |
bool rescanBlockchain() override; | |
void rescanBlockchainAsync() override; | |
void setAutoRefreshInterval(int millis) override; | |
int autoRefreshInterval() const override; | |
void setRefreshFromBlockHeight(uint64_t refresh_from_block_height) override; | |
uint64_t getRefreshFromBlockHeight() const override { return m_wallet->get_refresh_from_block_height(); }; | |
void setRecoveringFromSeed(bool recoveringFromSeed) override; | |
void setRecoveringFromDevice(bool recoveringFromDevice) override; | |
void setSubaddressLookahead(uint32_t major, uint32_t minor) override; | |
bool watchOnly() const override; | |
bool isDeterministic() const override; | |
bool rescanSpent() override; | |
NetworkType nettype() const override {return static_cast<NetworkType>(m_wallet->nettype());} | |
void hardForkInfo(uint8_t &version, uint64_t &earliest_height) const override; | |
bool useForkRules(uint8_t version, int64_t early_blocks) const override; |
Again same for make_uri()
below.
src/wallet/api/wallet2_api.h
Outdated
@@ -1066,8 +1066,10 @@ struct Wallet | |||
virtual bool verifyMessageWithPublicKey(const std::string &message, const std::string &publicKey, const std::string &signature) const = 0; | |||
|
|||
virtual bool parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error) = 0; | |||
virtual bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error); | |
virtual bool parse_uri(const std::string &uri, std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, std::string &payment_id, std::string &tx_description, std::vector<std::string> &unknown_parameters, std::string &error) = 0; |
= 0
at the end for "pure virtual" functions
src/wallet/api/wallet2_api.h
Outdated
virtual std::string make_uri(const std::string &address, const std::string &payment_id, uint64_t amount, const std::string &tx_description, const std::string &recipient_name, std::string &error) const = 0; | ||
|
||
virtual std::string make_uri(std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, const std::string &payment_id, const std::string &tx_description, std::string &error) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual std::string make_uri(std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, const std::string &payment_id, const std::string &tx_description, std::string &error) const; | |
virtual std::string make_uri(std::vector<std::string> &addresses, std::vector<std::uint64_t> &amounts, std::vector<std::string> &recipient_names, const std::string &payment_id, const std::string &tx_description, std::string &error) const = 0; |
tests/functional_tests/uri.py
Outdated
assert res.uri.address == address | ||
assert res.uri.payment_id == '' | ||
assert res.uri.amount == 0 | ||
assert res.uri.tx_description == '' | ||
assert res.uri.recipient_name == '' | ||
assert res.uri.payment_id == '' | ||
assert res.uri.tx_description == '' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several places where the order of asserts is changed, without changing the logic, these I'd consider unnecessary changes.
src/wallet/api/wallet.cpp
Outdated
bool WalletImpl::parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error) | ||
bool WalletImpl::parse_uri(const std::string& uri, std::string& address, std::string& payment_id, uint64_t& amount, std::string& tx_description, std::string& recipient_name, std::vector<std::string>& unknown_parameters, std::string& error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is also unnecessary
@SNeedlewoods I think I've addressed your issues and fixed them. Please check again if you are willing to, and tell me if I missed anything. As it seems, I've missed 2 unnecessary edits in |
The things I addressed look good to me now. Thanks for your contribution. I want to make clear though, I haven't looked into changes in |
I'm busy with the current release, will be able to take a look afterwards. |
@selsta ping . |
Like I said, I won't have time to re-review until v0.18.4.0 is released. The release has priority currently. |
There are over 200 open pull requests and we are working on a release. Contributing to open source software often requires patience, I hope you understand that. Anyway I appreciate you trying to work on this, even if you closed it now. |
This pull request addresses the issues raised in #7737 and introduces a new feature to the Monero codebase: multi-URL support for
parse_uri
andmake_uri
functions.Key Changes:
i
withj
for proper iteration).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.
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.