feat(extension-chrome): ownership service #76

zhangyouxin · 2023-02-13T05:43:46Z

usage:

// given backend, keystoreService, storage, notificationService

// probe task runs regularly in the background to guarantee that
// the lockInfos in storage is up to date with the CKB chain
// and make sure there are plenty of off-chain locks to use.

ProbeTask.getInstance().run();

const { fullOwnershipService, ruleBasedOwnershipService } = createOwnershipServices({
  backend,
  keystoreService,
  notificationService,
  locksManager: new LocksManager({ storage }),
});

fullOwnershipService.getOffChainLocks({});

all tx of lock

zhangyouxin · 2023-02-20T02:58:01Z

I reviewed the test cases related to full ownership, but to be honest, the completion of this PR is still too low

no paginated test case of getLiveCells

no matched test case of getOffChainLocks, only with a toHaveLength(1)

no non-empty test case of getOnCHainLocks

no non-empty test case of signTransaction

no non-empty test case of signData

these cases are probably the most important of this PR for a reviewer. As a reviewer, if there are some cases like this, it would be more friendly
describe('getLiveCells', async () => {
  it('should only the first page of cells will be fetched', () => {
    mockedBackend.pushCells([createCell({lock: key1}, createCell({lock: key2}))])
    probeTask.start();
    await sleep(100);
    expect(ownership.getLiveCells()).resolve.toMatchObject([
      createCell({lock: key1}, 
      createCell({lock: key2}))
    ])
  });

  it('should paginated when the cursor is non-empty', () => {
    const cells = [withLock1, withLock2, ...,  withLock15] 
    ...
    expect(ownership.getLiveCells()).resolve.toMatchObject(first10Cells);
    expect(ownership.getLiveCells({cursor: page2})).resolve.toMatchObject(last5Cells)
  })
})

added test cases mentioned above, and now the branch coverage is higher

packages/extension-chrome/__tests__/services/ownership/ownership.ts

homura · 2023-02-21T07:53:19Z

packages/extension-chrome/src/services/ownership/backend.ts

+  }
+}
+
+export class BackendProvider {


we should remove it because config is loaded from ConfigService

homura · 2023-02-21T07:56:45Z

packages/extension-chrome/src/services/ownership/backend.ts

+  nodeUri: string;
+  indexer: Indexer;
+  rpc: RPC;
+  indexerRPC: IndexerRPC;


these properties are unnecessary, which make it hard to test and limits support for the other backend like the CKB Light Client in the future

homura · 2023-02-21T07:59:17Z

packages/extension-chrome/src/services/ownership/backend.ts

+      indexerCursor?: string;
+    };
+  }) => Promise<Paginate<Cell>>;
+  getLiveCellFetcher: () => (outPoint: OutPoint) => Promise<Cell>;


getLiveCell(outPoint: OutPoint): Promise<Cell> is enough

homura · 2023-02-21T08:06:29Z

packages/extension-chrome/src/services/ownership/backend.ts

+  rpc: RPC;
+  indexerRPC: IndexerRPC;
+
+  hasHistory: (payload: { lock: Script }) => Promise<boolean>;


Should be hasHistories(payload: { lock: Script }): Promise<boolean[]>
fetch histories without batching probably cause performance issue

homura · 2023-02-21T08:43:41Z

packages/extension-chrome/src/services/ownershipService.ts

+    },
+  };
+}
+export async function getOnchainLockProvider(


should be getOnChainLockProvider

homura · 2023-02-21T08:59:14Z

packages/extension-chrome/src/services/ownership/locksManager.ts

+    });
+  }
+
+  async getlockInfoByLock({ lock }: { lock: Script }): Promise<LockInfo | undefined> {


could be getLockInfoByLock

homura · 2023-02-21T09:22:14Z

packages/extension-chrome/src/services/ownershipService.ts

+      const lockInfo = await config.locksManager.getlockInfoByLock({ lock: payload.lock });
+      asserts.asserts(lockInfo, 'Lock not found when call signData with', payload);
+      const password = (await config.notificationService.requestSignData({ data: payload.data })).password;
+      const signature = await config.keystoreService.signMessage({


to avoid security risk, the signData must with some magick bytes, we can mark a TODO here

homura · 2023-02-21T11:08:08Z

packages/extension-chrome/src/rpc/server.ts

+  const keystoreService = createServicesFactory().get('keystoreService');
+  const storage = createServicesFactory().get('storage');
+  if (!keystoreService || !storage) return;
+  console.log('wallet already initialized, start probe task...');


we can use the logger instead of console.log

homura · 2023-02-21T11:10:00Z

packages/extension-chrome/src/rpc/server.ts

+  const keystoreService = createServicesFactory().get('keystoreService');
+  const storage = createServicesFactory().get('storage');


refactor the keystoreSertvice and storage to params instead of createServiceFactory everywhere

homura · 2023-02-21T11:12:58Z

packages/extension-chrome/src/rpc/types.ts


 export interface WalletMethods extends CallMap {
  wallet_enable: Call<void, void>;
  wallet_isEnabled: Call<void, boolean>;
  wallet_getNetworkName: Call<void, string>;

-  wallet_fullOwnership_getUnusedLocks: Call<void, Script[]>;
+  wallet_fullOwnership_getOffChainLocks: Call<GetOffChainLocksPayload, Promise<Script[]>>;


Promise is unnecessary in Call

homura · 2023-02-21T11:14:17Z

packages/extension-chrome/src/rpc/walletImpl.ts

@@ -24,15 +30,121 @@ addMethod('wallet_enable', async (_, { getRequesterAppInfo, resolveService }) =>
  }

  await configService.addWhitelistItem({ host: host, favicon: `${protocol}//${host}/favicon.ico` });
+  const servicesFactory = createServicesFactory();


use resolveService instead of createServicesFactory() everywhere

homura · 2023-02-21T11:16:07Z

packages/extension-chrome/src/rpc/walletImpl.ts

+});
+
+addMethod('wallet_fullOwnership_getOffChainLocks', async (payload) => {
+  console.log('====================================');


====== is unnecessary, if you just want to make a different logger, please createLogger('special_module_name') to instead

homura · 2023-02-21T11:16:32Z

packages/extension-chrome/src/rpc/walletImpl.ts

+  const keystoreService = await servicesFactory.get('keystoreService');
+  const notificationService = await servicesFactory.get('notificationService');
+  const storage = await servicesFactory.get('storage');
+  console.log('storage', await storage.getItem('full' as never));


avoid using never as a parameter, it is not recommendded

homura · 2023-02-21T13:02:42Z

This PR has many issues, and we should plan to reimplement this PR

Complex Asynchronous

Ownership need to handle many async, this PR uses Promise.all and for + Promise to implement async. When we need to control the concurrency of async, this would be a bad solution. For example, when we need to probe if the next 100 on-chain locks have cells or not, Promise.all may cause the server to reject too many connection requests

Not Enough Abstraction

The full-external full-internal rule-based are similar, and we don't need to treat them differently, we can just use compose instead of the cross these condition like getRuleBasedOffChainLocks

interface DerivedLocks {
  lockInfos: LockInfo[];
}

const isFullOwnership = (info) => info.parantPath === FULL_OWNERSHIP_PARENT_PATH;
const isRuleBasedOwnership = (info) => info.parentPath === RULE_BASED_OWNERSHIP_PARENT_PATH;
const isOnChain = (info) => info.change === 'external';
const ifOffChain = (info) => info.change === 'internal';

// an async db driver
const infos = await lockInfos.filter(compose(isFullOwnership, isOffChain));
...

Therefor, we will not merge this PR, but we will use this branch for writing e2e cases

Signed-off-by: homura <homura.dev@gmail.com>

…-ownership

zhangyouxin added 30 commits January 28, 2023 11:06

add get used lock for ownership service

708c638

add signData function in ownershipService

bf67f0b

add addressStorage in createOwnershipService parameter

0bc7acf

update used locks list in addressStorage service

fc70d90

add test case for sign data

491183a

refactor: use service interfaces

948de5d

refactor: collect only one tx when dectect active lock to avoid reading

9f3d28f

all tx of lock

refactor: use payload in service API parameters

372213c

refactore: reorgnize addressStorage service code

89894d6

refactor: change backend service name to backend.ts

1fd834f

refactor: reorgnize backend service code

c328292

feat: implement getLiveCells of ownership service

8631d79

refactor: reorgnize test cases

6c8dd5e

test: add test case for get live cells

3c1b717

feat: implement sign transaction

28b6e96

test: add test case for get grouped message

7b1dcf3

test: add test case for sign trasaction

2e7f300

test: add test case for addressStorage and remove unused code

1212d09

chore: change to the comments

a2adc4e

refactor: use bytes.equal to compare hashes

668c0fd

refactor: change getExtendedPublicKey to getPublicKeyByPath

17f373a

refactor: sync all addressInfo when getting unused locks

1a793f3

refactor: use createTransactionSkeleton rather than implement it

1e6b8ab

refactor: change ownershipService name to fullOwnershipService

3a0aa40

refactor: reorgnize full ownership tests

9181fb2

feat: add fullOwnership addressStorage service

ad822d8

feat: create full/ruleBased ownership service

6651db1

test: add test for address storage service

a67fb7c

refactor: rename used/unused to on/off chain locks

6287a6d

feat: use new design of rule based path m/ckb_purpose 1'/0'

7c1bc88

test: add test case for rule based ownership

cc84f60

zhangyouxin force-pushed the rule-based-ownership branch from 41fcc72 to cc84f60 Compare February 19, 2023 12:56

refactor: backend getLiveCell return paginated results

6476649

homura reviewed Feb 20, 2023

View reviewed changes

packages/extension-chrome/__tests__/services/ownership/ownership.ts Outdated Show resolved Hide resolved

test: modify test locks data to diffrent

e0deef2

homura mentioned this pull request Feb 20, 2023

2023-02-20 #80

Closed

11 tasks

zhangyouxin added 2 commits February 21, 2023 10:58

Merge branch 'main' into rule-based-ownership

fb1bf0f

test: manually test on ckb-testnet and fix known issue

5cba1bb

homura reviewed Feb 21, 2023

View reviewed changes

zhangyouxin and others added 12 commits February 22, 2023 12:24

test: fix tests

8cf2c52

chore: joint test with demo-nexus-dapp

284d1f8

fix: avoid log 0x for empty array

a9ca560

Signed-off-by: homura <homura.dev@gmail.com>

docs: start extension

a0488bb

Signed-off-by: homura <homura.dev@gmail.com>

refactor: make rpc testable

f70e987

Signed-off-by: homura <homura.dev@gmail.com>

chore: remove some expired todos

53b5722

Signed-off-by: homura <homura.dev@gmail.com>

feat: localhost should be able to be added to whitelist

4ba619a

test: unnecessary mock

b5b4db9

Signed-off-by: homura <homura.dev@gmail.com>

refactor: simplify createServer

d88ea86

Signed-off-by: homura <homura.dev@gmail.com>

test: remove unnecessary clearMock

b0d49a3

Signed-off-by: homura <homura.dev@gmail.com>

fix: fix test case with config validation err

bb5fa34

Merge remote-tracking branch 'upstream/rpc-unit-test' into rule-based…

ce4f107

…-ownership

zhangyouxin force-pushed the rule-based-ownership branch from 9812f54 to ce4f107 Compare February 24, 2023 06:52

zhangyouxin added 3 commits February 24, 2023 16:17

test: stop probing task after test cases run

6976d44

Merge branch 'joi-localhost' into rule-based-ownership

40c9ebe

fix: add type in getLiveCells

97f1935

homura mentioned this pull request Feb 27, 2023

feat: implement ownership #89

Merged

zhangyouxin closed this Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(extension-chrome): ownership service #76

feat(extension-chrome): ownership service #76

zhangyouxin commented Feb 13, 2023 •

edited

Loading

zhangyouxin commented Feb 20, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura Feb 21, 2023

homura commented Feb 21, 2023

		const keystoreService = createServicesFactory().get('keystoreService');
		const storage = createServicesFactory().get('storage');

feat(extension-chrome): ownership service #76

feat(extension-chrome): ownership service #76

Conversation

zhangyouxin commented Feb 13, 2023 • edited Loading

zhangyouxin commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homura commented Feb 21, 2023

Complex Asynchronous

Not Enough Abstraction

zhangyouxin commented Feb 13, 2023 •

edited

Loading