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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

knst
Copy link
Collaborator

@knst knst commented May 18, 2024

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 #5965 which has not been released yet.

Checklist:

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

@knst knst added this to the 21 milestone May 18, 2024
@UdjinM6
Copy link

UdjinM6 commented May 18, 2024

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

@knst
Copy link
Collaborator Author

knst commented May 18, 2024

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
Copy link

UdjinM6 commented May 18, 2024

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
Copy link
Collaborator Author

knst commented May 18, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

knst commented May 19, 2024 •
edited
Loading

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.

@knst knst closed this May 19, 2024
@shumkov shumkov mentioned this pull request Jun 14, 2024
Merged
@knst knst mentioned this pull request Jul 5, 2024
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Jul 8, 2024
@PastaPastaPasta
Merge #6095: fix: createwallet to require 'load_on_startup' for descr…
50a3b7c 
…iptor wallets

a42e9df fix: createwallet to require 'load_on_startup' for descriptor wallets (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPC `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` since #5965
  `load_on_startup` used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected.

  See also previous attempt to resolve issue: #6029

  ## What was done?
  To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21.

  ## How Has This Been Tested?
  Run unit/functional tests

  Tested with CLI:
  ```
  $ createwallet "tmp-create" true true "" false true
  RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8)
  $ createwallet "tmp-create" true true "" false true true
  {
    "name": "tmp-create",
    "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet"
  }
  ```

  ## Breaking Changes
  You can't more pass 'descriptor=NN' without  #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

ACKs for top commit:
  UdjinM6:
    utACK a42e9df
  PastaPastaPasta:
    utACK a42e9df
  thephez:
    utACK a42e9df

Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2024
@PastaPastaPasta
Merge dashpay#6095: fix: createwallet to require 'load_on_startup' fo…
cdf7a25 
…r descriptor wallets

a42e9df fix: createwallet to require 'load_on_startup' for descriptor wallets (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPC `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` since dashpay#5965
  `load_on_startup` used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected.

  See also previous attempt to resolve issue: dashpay#6029

  ## What was done?
  To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21.

  ## How Has This Been Tested?
  Run unit/functional tests

  Tested with CLI:
  ```
  $ createwallet "tmp-create" true true "" false true
  RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8)
  $ createwallet "tmp-create" true true "" false true true
  {
    "name": "tmp-create",
    "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet"
  }
  ```

  ## Breaking Changes
  You can't more pass 'descriptor=NN' without  dashpay#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

ACKs for top commit:
  UdjinM6:
    utACK a42e9df
  PastaPastaPasta:
    utACK a42e9df
  thephez:
    utACK a42e9df

Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta Awaiting requested review from PastaPastaPasta

@UdjinM6 UdjinM6 Awaiting requested review from UdjinM6

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@knst @UdjinM6 @PastaPastaPasta