fix!: reverse flags "descriptor" and "load_on_startup" on RPC createwallet

knst commented 2 months ago

Issue being fixed or feature implemented

PR with experimental support of descriptor wallets introduced breaking changes:

createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup ) load_on_startup used to be an argument 5 but now has a number 6.

Even if this breaking changes reduces possible future conflicts with bitcoin, it's easy to avoid this breaking changes. Especially, because both types are "bool" an user may even not notice that something wrong when he meant "load on startup" but got "descriptor wallet" which is not expected.

Happiness of thousands of users over-weight possible future backporting inconvenience.

What was done?

Reversed arguments 5 and 6 for RPC createwallet: ... descriptors, load_on_startup -> ... load_on_startup, descriptors

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

It revers breaking changes from https://github.com/dashpay/dash/pull/5965 which has not been released yet.

Checklist:

[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have added or updated relevant unit/integration/functional/e2e tests
[ ] I have made corresponding changes to the documentation
[x] I have assigned this pull request to a milestone

UdjinM6 commented 2 months ago

Code looks good but DNM imo - it's perfectly fine to break API in major versions.

knst commented 2 months ago

it's perfectly fine to break API in major versions.

As I mentioned in PR description:

Even if this breaking changes reduces possible future conflicts with bitcoin, it's easy to avoid this breaking changes.
Especially, because both types are "bool" an user may even not notice that something wrong when he meant "load on startup" but got "descriptor wallet" which is not expected.

Happiness of thousands of users over-weight possible future backporting inconvenience.

So, firstly, type of these arguments are same (boolean), so, anyone who will call createwallet without being aware of breaking changes - can get unexpected behavior of app and even not realize it but got not what he wants and spend significant time to figure our an issue. I consider happiness of users as a bigger priority than happiness of developer who is doing backports from bitcoin.

Secondly, this breaking changes (moving load_on_startup same as bitcoin) doesn't give any benefits for users point of view, for developer point of view:

it doesn't improve API
it doesn't make the implementation shorter or easier
it doesn't improve performance
etc - no any benefits except possible minor conflicts when backporting.

So, I would have this one merged

UdjinM6 commented 2 months ago

it's perfectly fine to break API in major versions. ... Secondly, this breaking changes (moving load_on_startup same as bitcoin) doesn't give any benefits for users point of view, for developer point of view:

it doesn't improve API ...

etc - no any benefits except possible minor conflicts when backporting.

well, it keeps API bitcoin-compatible which is definitely a benefit imo for cross-blockchain apps like multi-coin wallets etc.

knst commented 2 months ago

well, it keeps API bitcoin-compatible which is definitely a benefit imo for cross-blockchain apps like multi-coin wallets etc.

it makes sense, I will agree that bitcoin-like compatibility of API is a good.

@PastaPastaPasta what do you think?

PastaPastaPasta commented 2 months ago

Given that bitcoin also has both descriptors and load_on_startup, I think it is more beneficial to match their api: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/client.cpp#L301C1-L303C1

knst commented 2 months ago

even re-ordering load_on_startup is breaking changes, it stays as it is now for sack of better bitcoin-like compatibility. So, bitcoin's docs and tutorials, multi-currency wallets, etc are matched with our RPC createwallet and bitcoin's RPC as discussed.

dashpay / dash