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

Normalize wallet RPC commands (refillkeypool, unlockwallet, lockwallet, setpassphrase) #1983

Closed
wants to merge 1 commit into from

Conversation

kjj2
Copy link

@kjj2 kjj2 commented Nov 5, 2012

Updates 4 odd RPC commands to match other RPC commands which tend to follow a naming scheme.

The old names still work as aliases of the new names, but don't show up in the help lists.

See: https://bitcointalk.org/index.php?topic=122345.0

@jgarzik
Copy link
Contributor

jgarzik commented Nov 5, 2012

I like the old, deprecated names much better than the new ones ;p

@Diapolo
Copy link

Diapolo commented Nov 5, 2012

IMHO, we should define, what deprecated means in that case and when it is considered safe to remove old naming conventions :). Perhaps add a date or version, when this is the case.

@gavinandresen
Copy link
Contributor

ENOCARE/ACK.

(I don't really care, changes looks fine).

@laanwj
Copy link
Member

laanwj commented Nov 6, 2012

Have we really arrived at the point that we deprecate commands (and thus break backwards compatibility) for aesthetic reasons?

This means we'll end up with duplicate commands for a long time, make it harder for people to google the commands. And communicating this to users is another problem, as we don't have an official API documentation to say "this will be deprecated in version XXX".

It's not worth the trouble.

@Diapolo
Copy link

Diapolo commented Nov 6, 2012

I just wanted to ask :), your points indeed are clear and valid! As this pull renames without removing the old names that problem doesn't rise.

@sipa
Copy link
Member

sipa commented Nov 6, 2012

I think the only RPC makeover worth doing is a much more fundamental one, where we normalize commands, separate them clearly into modules (as already reflected in the source code... but who would now guess that gettransaction is a wallet RPC, and getrawtransaction a blockchain one?), normalize the data types used (amount as strings/satoshis/floats, difficulty as targethash/hexbits/float), perhaps add support for multiple wallets, make the error codes consistent, ...

That is much more work though, and probably means some RPC v2 mechanism like was already proposed before. About this... -ENOCARE

@gavinandresen
Copy link
Contributor

@laanwj : "setaccount" used to be called "setlabel", so there is a precedent for breaking compatibility (after maintaing the old names for a while) just for aesthetic reasons.

@jgarzik
Copy link
Contributor

jgarzik commented Nov 16, 2012

Consensus seems to be "don't care / meh / not worth the trouble"... closing.

Perhaps put this on a list for a bigger RPC revamp, or right before RPC is locked in stone for version 1.0 (whenever that is).

@jgarzik jgarzik closed this Nov 16, 2012
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
…l test framework

5181327 [Tests][Cleanup] unused zerocoin specifics in the framework (random-zebra)
f30ede7 [Tests] Don't generate PoS cache (random-zebra)
abb7b11 [Tests] Don't use POS cache in mining_pos_reorg test (random-zebra)
b731987 [Tests] Remove zPIV spend test and balance checks (random-zebra)

Pull request description:

  This greatly speeds up the global test runtime, as we no longer need to create a secondary "pos" cache.

ACKs for top commit:
  furszy:
    utACK 5181327.

Tree-SHA512: 3a11e75025df3ab30941f31fc207e2ce4c87ddfd48c99507e2ac2fc828f701453f24c5e1100ff3d4aec09251e0b67d12916133563ad43e5c568ee9485fefb7ec
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
ab9c8d0 [RPC] Add totalsupply/moneysupply to getsupplyinfo/getinfo (random-zebra)
cea10c7 [RPC][Cleanup] Remove zPIVsupply/zerocoinbalance from getinfo (random-zebra)
c764f02 [RPC] Add shieldedsupply to getsupplyinfo and getinfo (random-zebra)

Pull request description:

  Add the shielded pool value (at the chain tip) to `getsupplyinfo`/`getinfo` output.
  Rename `moneysupply` --> `transparentsupply`
  Remove `zPIVsupply`/`zerocoinbalance` from `getinfo` output.

  Based on top of:
  - [x] bitcoin#1983

ACKs for top commit:
  furszy:
    ACK ab9c8d0.

Tree-SHA512: 62bda77195f8ae4c26efe4da417e850790c522ec3ccde39cc6f13ea4448947d4b1321b31201da8813923aa87640283cd913e40c3f45cce4dd8565ff8aae8924b
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
e6ca43b [Cleanup] Remove reindexzerocoin init option (random-zebra)
c340155 [Cleanup][Tests] Remove zerocoin unit tests (random-zebra)
7ba49eb [Cleanup] Remove denomination_functions and mapZerocoinSupply (random-zebra)
9485b49 [Cleanup] Cleanup miner from zerocoin-txes management (random-zebra)
e99388e [Refactor] Remove direct constructor for public spends (random-zebra)
8a30aac Trivial: remove unused files (random-zebra)
e0b8196 Cleanup: Remove zerocoin wallet and tracker (random-zebra)
c1d56b8 Cleanup: cleanup zPIV transaction records in the GUI (random-zebra)
9e7c76b [RPC][Cleanup] Remove zerocoin supply/balance (random-zebra)
ad7a532 [RPC][Cleanup] Remove zerocoin RPC functions (random-zebra)

Pull request description:

  Keep going with the zerocoin cleanup.
  This time:
  - RPC
  - wallet
  - tracker
  - unit tests
  - supply map
  - init flags

  Based on top of
  - [x] bitcoin#1983

  To be merged **after** zerocoin-maintenance has been activated via spork on main net.

ACKs for top commit:
  Fuzzbawls:
    ACK e6ca43b
  furszy:
    ACK e6ca43b .

Tree-SHA512: 062dabca6fce3896a9daf6b74d53f332e5b849686cab45f312767df1f929733e97c9db4f5595de0b8fec0b7c231aed9a23b9afb96db95ad4cdd58e84f836c140
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants