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.

Sign up for GitHub

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

Jump to bottom

wallet, simplewallet: Drop support for mine-to-use RPC system #8724

Merged
merged 1 commit into from
Apr 25, 2023
Merged

wallet, simplewallet: Drop support for mine-to-use RPC system #8724

merged 1 commit into from
Apr 25, 2023

Conversation

jeffro256
Copy link
Contributor

The main thing to test here is new wallet UX with old RPC mine-to-use node. Using post-PR wallet with pre-PR node should generate error message specific to this PR and not generic "command failed" mesages.

@jeffro256
Copy link
Contributor Author

#8722

@plowsof
Copy link
Contributor

plowsof commented Feb 2, 2023

New wallet connecting old rpc mine-to-use node error msg: (i guess the refresh thread line could be dropped, and im not sure about the See . Block received: 0 being on the same line)

Starting refresh...
Error: refresh failed: Daemon requires deprecated RPC payment. See . Blocks received: 0
Background refresh thread started
[wallet 437Jhp (out of sync)]:

👍

@jeffro256
Copy link
Contributor Author

@plowsof Thanks for testing. That error message was a typo, I meant to put the link to the relevant GH issue.

@plowsof plowsof mentioned this pull request Feb 2, 2023
Closed
@jeffro256 jeffro256 mentioned this pull request Feb 6, 2023
Closed
rbrunner7
rbrunner7 reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@rbrunner7 rbrunner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a go at it. Pretty clear of course because mostly deleting stuff.

src/wallet/wallet2.h Outdated Show resolved Hide resolved
src/wallet/wallet2.h Outdated Show resolved Hide resolved
src/wallet/wallet_rpc_server.cpp
@@ -4517,7 +4517,6 @@ class t_daemon

const auto arg_wallet_file = wallet_args::arg_wallet_file();
const auto arg_from_json = wallet_args::arg_generate_from_json();
const auto arg_rpc_client_secret_key = wallet_args::arg_rpc_client_secret_key();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused: If I checked correctly you let a lot of RPC payment related stuff stand in the wallet RPC server, but out all possible things to delete you only delete things related to this key.

If that's really the case, what's the idea here?

src/simplewallet/simplewallet.cpp Show resolved Hide resolved
src/wallet/wallet2.cpp Outdated Show resolved Hide resolved
src/wallet/wallet2.cpp Show resolved Hide resolved
@jeffro256
Copy link
Contributor Author

@rbrunner7 Thanks for reviewing! I updated the PR with some of your comments. What did you mean by "you let a lot of RPC payment related stuff stand in the wallet RPC server"? Would you please be able to point me to that?

@rbrunner7
Copy link
Contributor

Would you please be able to point me to that?

Well, I did a simple search for "rpc_pay" in the file wallet_rpc_server.cpp and got quite a number of hits, in methods that seem to implement RPC pay related commands, e.g. COMMAND_RPC_GET_BULK_PAYMENTS or COMMAND_RPC_GET_PAYMENTS.

@jeffro256
Copy link
Contributor Author

IIUC, those RPC endpoints are unrelated to the mine-to-use system. AFAIK, those are RPC commands for retrieving general purpose payment information.

@rbrunner7
Copy link
Contributor

(Slaps forehead) Of course, you are right, that's purely coincidence that the string "rpc_pay" results there.

But then I am still a bit confused: How can it be that the RPC server has (almost) no RPC Pay related code in it? Does that mean that it doesn't implement that? Or is it enough for it to work that merely wallet2 cares about it?

@jeffro256
Copy link
Contributor Author

Haha... there's certain words that don't seem like words to me anymore: transaction, payment, mine, access, daemon, etc. They all kinda blur together.

Does that mean that it doesn't implement that?

Yes, I believe so. I really think the only wallet that supported mine-to-use was simplewallet, and not by default either. wallet2 had most of the code for mine-to-use, but there was also a non-trivial amount of logic in simplewallet which managed the process.

rbrunner7
rbrunner7 approved these changes Feb 12, 2023
View reviewed changes
Copy link
Contributor

@rbrunner7 rbrunner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Let's shed a last tear for all the good code that mooo implemented for this feature, and enjoy the simplified wallet code :)

tobtoht
tobtoht approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@tobtoht tobtoht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Thank you for working on this.

@jeffro256 jeffro256 force-pushed the rm_rpc_payment_access_wallet branch from 34f0726 to ed16f53 Compare February 26, 2023 20:04
@jeffro256
Copy link
Contributor Author

Squashed and rebased against master

@jeffro256 jeffro256 force-pushed the rm_rpc_payment_access_wallet branch from ed16f53 to b7820e4 Compare March 19, 2023 03:12
@jeffro256
Copy link
Contributor Author

Rebased against master again to resolve conflicts with #8076

@rbrunner7
Copy link
Contributor

I had a look at the rebase changes as far as they merge code in from my own #8076 (probably the largest single source of conflicts), and compiled the rebased code for a quick check whether pool handling still works.

I could not find any problems, this gets an "ok" from me.

@plowsof plowsof mentioned this pull request Mar 24, 2023
Open
@selsta selsta mentioned this pull request Apr 15, 2023
Closed
@jeffro256
wallet, simplewallet: Drop support for mine-to-use RPC system
d2a591d 
Using post-PR wallet with pre-PR node will generate error message specific to this PR and not generic "command failed" mesages.
@jeffro256 jeffro256 force-pushed the rm_rpc_payment_access_wallet branch from b7820e4 to d2a591d Compare April 25, 2023 15:18
@jeffro256
Copy link
Contributor Author

Rebased against master again to fix merge conflicts with #8698

@jeffro256
Copy link
Contributor Author

To verify rebase:

git show b7820e4 > a.txt
git show d2a591d > b.txt
diff a.txt b.txt

@luigi1111 luigi1111 merged commit 1c9686c into monero-project:master Apr 25, 2023
@jeffro256 jeffro256 mentioned this pull request May 8, 2023
Closed
@UkoeHB UkoeHB mentioned this pull request May 23, 2023
Open
@Gingeropolous Gingeropolous mentioned this pull request Aug 26, 2023
Closed
@HardenedSteel HardenedSteel mentioned this pull request Mar 9, 2024
Closed
@selsta selsta mentioned this pull request Mar 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobtoht tobtoht tobtoht approved these changes

@rbrunner7 rbrunner7 rbrunner7 approved these changes

@selsta selsta selsta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jeffro256 @plowsof @rbrunner7 @selsta @tobtoht @luigi1111