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

RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint wallet #20448

Merged
merged 1 commit into from Nov 28, 2020

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Nov 21, 2020

Allow specifying the wallet_name param to unloadwallet on RPC wallet endpoints, so long as it matches the endpoint wallet.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

FWIW I thought about disallowing wallet_name when using a wallet endpoint.

@luke-jr
Copy link
Member Author

luke-jr commented Nov 21, 2020

That's what the current code does (and the first commit of this PR documents).

@maflcko
Copy link
Member

maflcko commented Nov 23, 2020

would be easier to review and backport if the commit was a separate pull

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Approach ACK

@@ -355,12 +355,18 @@ def wallet_file(name):
assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[0].unloadwallet)
assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", self.nodes[0].unloadwallet, "dummy")
assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", node.get_wallet_rpc("dummy").unloadwallet)
assert_raises_rpc_error(-8, "Cannot unload the requested wallet", w1.unloadwallet, "w2"),
assert_raises_rpc_error(-8, "RPC request wallet and wallet_name parameter specify different wallets", w1.unloadwallet, "w2"),
Copy link
Contributor

@jonatack jonatack Nov 23, 2020

Choose a reason for hiding this comment

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

81162ec For the first commit, suggest testing with the same wallet name:

-        assert_raises_rpc_error(-8, "Both the RPC request wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, "w2"),
+        assert_raises_rpc_error(-8, "Both the RPC request wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, w1.getwalletinfo()["walletname"]),

src/wallet/rpcwallet.cpp Show resolved Hide resolved
@luke-jr
Copy link
Member Author

luke-jr commented Nov 23, 2020

@MarcoFalke Should I interpret that as a NACK to backporting the 2nd commit? (I said at least the first...)

@maflcko
Copy link
Member

maflcko commented Nov 23, 2020

Yes, feature freeze was a month ago

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 23, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

maflcko pushed a commit that referenced this pull request Nov 24, 2020
…the RPC request and wallet_name parameter specify a wallet

b1f59d5 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)

Pull request description:

  Just documentation clarifications from #20448

ACKs for top commit:
  MarcoFalke:
    review ACK b1f59d5
  jonatack:
    re-ACK b1f59d5  per `git diff e8303a0 b1f59d5`

Tree-SHA512: ac068b0aa7ceed49496367fdd9425b59dbba18b56e89b26afc22a6c8ece51f0b92a169cacd55740b1cadab2b32f4f8e8700e609066ab7e59d3b53c7891da585e
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 24, 2020
…n both the RPC request and wallet_name parameter specify a wallet

b1f59d5 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)

Pull request description:

  Just documentation clarifications from bitcoin#20448

ACKs for top commit:
  MarcoFalke:
    review ACK b1f59d5
  jonatack:
    re-ACK b1f59d5  per `git diff e8303a0 b1f59d5`

Tree-SHA512: ac068b0aa7ceed49496367fdd9425b59dbba18b56e89b26afc22a6c8ece51f0b92a169cacd55740b1cadab2b32f4f8e8700e609066ab7e59d3b53c7891da585e
@luke-jr luke-jr changed the title RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC request, and document behaviour RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC request Nov 24, 2020
@luke-jr luke-jr changed the title RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC request RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint wallet Nov 24, 2020
@luke-jr
Copy link
Member Author

luke-jr commented Nov 24, 2020

rebased

@jonatack
Copy link
Contributor

ACK 89bdad5

@maflcko
Copy link
Member

maflcko commented Nov 28, 2020

review ACK 89bdad5

@maflcko maflcko merged commit 1ae5758 into bitcoin:master Nov 28, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 28, 2020
…t_name param matching RPC endpoint wallet

89bdad5 RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint (Luke Dashjr)

Pull request description:

  Allow specifying the `wallet_name` param to `unloadwallet` on RPC wallet endpoints, so long as it matches the endpoint wallet.

ACKs for top commit:
  jonatack:
    ACK 89bdad5
  MarcoFalke:
    review ACK 89bdad5

Tree-SHA512: efb399c33f7b5596870a26a8680f453ca47aa7a6db4e550f9435d13044f1c4bad0ae11e8f0205213409d08b75c4188c3be782e54aafab1f65b97eb8cf5c252a9
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants