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

feat: split business logic into services #44

Merged
merged 15 commits into from
Feb 2, 2023
Merged

feat: split business logic into services #44

merged 15 commits into from
Feb 2, 2023

Conversation

homura
Copy link
Contributor

@homura homura commented Jan 16, 2023

Resolved #29

This PR dependent on #39, to

  1. split business logic into several services
  2. organize services and RPC server
  3. show how to implement service and how to organize services, check out the wallet_enable in rpc/walletImpl.ts

TODO

@homura homura marked this pull request as draft January 16, 2023 05:06
@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Merging #44 (e72fc03) into main (aa2cae8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #44   +/-   ##
=======================================
  Coverage   76.92%   76.92%           
=======================================
  Files           6        6           
  Lines          52       52           
  Branches       12       12           
=======================================
  Hits           40       40           
  Partials       12       12           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@homura homura mentioned this pull request Jan 16, 2023
9 tasks
Signed-off-by: homura <homura.dev@gmail.com>
Base automatically changed from json-rpc-2.0 to main January 28, 2023 09:55
@homura homura marked this pull request as ready for review January 30, 2023 04:48
@homura homura mentioned this pull request Jan 30, 2023
2 tasks
Signed-off-by: homura <homura.dev@gmail.com>
Signed-off-by: homura <homura.dev@gmail.com>
Signed-off-by: homura <homura.dev@gmail.com>
Signed-off-by: homura <homura.dev@gmail.com>
Comment on lines +47 to +54

/**
* non-hardened derivation path,
* the corresponding public key will be stored in plain text,
* for example, the BIP-44 path `m / 44'/ 309'/ 0'/ 0 / 0` will store the public key of
* `m / 44' / 309' / 0' / 0` (external) and `m/44'/ 309'/ 0'/ 1` (internal) in plain text
*/
paths: NonHardenedPath[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a scenario where user would pass in some path other than first external/change address, i.e. m / 44'/ 309'/ 0'/ 0 / 0 when calling initKeyStore? or this parameter only makes it more covenient for tests.

I think when a user creates a new wallet, he would only pass in password and mnemonic,
then getUsedLocks in ownershipService would be called to detect used locks and paths. So maybe move paths to OwnershipService

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the user you're referring to a Nexus contributor, or an end user of Nexus product?

Copy link
Contributor

Choose a reason for hiding this comment

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

A user of Nexus Wallet

Copy link
Contributor Author

@homura homura Jan 31, 2023

Choose a reason for hiding this comment

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

I got it. A service can't be called by an end user of Nexus directly, it is just a way for organizing code.

Nexus cannot directly access the user's private key unless the user grants permission and enters the password for the WSS(Web3 Secret Storage). To derive addresses without a private key, we can use a non-hardened extended public key. For security reason, we don't encourage other modules to have direct access to the keystore or even the extended public key.

To initialize the keystore, we can use these "parent" path

  • full-ownership (change: false): m/44'/309'/0'/0
  • full-ownership (change: true): m/44'/309'/0'/1
  • rule-based-ownership (change: false): m/49'/309'/0'/0
  • rule-based-ownership (change: true): m/49'/309'/0'/1
await keystore.initKeystore({
  paths: [`m/44'/309'/0'/0`, `m/44'/309'/0'/1`, `m/49'/309'/0'/0`, `m/49'/309'/0'/1` ]
})

// derived from       `m/44'/309'/0'/0`
keystore.getPublickey(`m/44'/309'/0'/0/0`)
keystore.getPublickey(`m/44'/309'/0'/0/1`)
keystore.getPublickey(`m/44'/309'/0'/0/3`)

/**
* derivation path of the private key, will be used to sign the message
*/
path: HardenedPath | NonHardenedPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

HardenedPath should not be used to sign a message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there anything wrong with using hardened path to sign messages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing wrong with that, just in most cases the user(nexus contributor) would call signData with SignDataPayload, which is constructed of data and script, but A hardend path does'nt produce a lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can only say that we won't use a hardened key to sign the message "now".

We don't restrict which key must be used to sign, maybe there will be a proposal to identify the user, for example, using m / identity', there are some scenarios that actually require for identity, such as voting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it. But in that case(using hardend path in voting Dapp), let's say using m/44'/309'/0' to sign a vote, the voting dapp will know every address of user(end user of nexus wallet), the privacy of UTXO doesn't exist any more.

Copy link
Contributor Author

@homura homura Jan 31, 2023

Choose a reason for hiding this comment

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

let's say using m/44'/309'/0' to sign a vote

The hardened key can only derive the child key from the parent private key, we can't derive the child key from m/44'/309'/0''s public key

And this is why only the NonhardenedPath can be access in keystore.initKeystore

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I get it, I just misunderstood the crypto algorithm here.

How can the Voting Dapp know the weight of a ticket signed by m/identity' if the voting weight depends on the balance of signer's coin balance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The voting app is just one example of a dApp that might require an identity mechanism, and I think we can discuss how to calculate voting weight in another issue

}

interface GetUnusedLocksPayload {
change?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

change lock should be used only when user issues a new transaction, so unused change locks should not be used in future transactions, maybe we don't need this option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all unused locks can be used to assemble a transaction, and Nexus should expose an API for dApp to access them, including the change lock. let's imagine a simple scenario of transferring CKB

inputs:
  - used_lock0: getLivecells[0]
  - used_lock1: getLivecells[1]
  - ...
outputs:
  - unused_lock0(change): getUnusedLocks({ change: true })[0];
  - target_lock: receiverLock

Copy link
Contributor

Choose a reason for hiding this comment

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

According to bip-44, internal chains receive only coins that come from the associated external chains.

I think that means if the address index is n, then m/44'/309'/1/n should be used in outputs only when m/44'/309'/0/n is in the inputs of the transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you misunderstood here, if you say change address can only be used in output, these change coin will never be spent anymore

@homura homura mentioned this pull request Jan 31, 2023
7 tasks
Copy link
Contributor

@zhangyouxin zhangyouxin left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: homura <homura.dev@gmail.com>
@homura homura merged commit a07af9b into main Feb 2, 2023
@homura homura deleted the services branch February 2, 2023 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

service interface design and organization
4 participants