refactor: remove dependency on indy ledger service from sov did resolver/registrar #1086

sairanjit · 2022-11-06T07:54:57Z

resolves: #1066 and #1067
Signed-off-by: Sai Ranjit Tummalapalli [email protected]

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

codecov-commenter · 2022-11-06T09:22:52Z

Codecov Report

Merging #1086 (224f98d) into main (574e6a6) will decrease coverage by 0.15%.
The diff coverage is 86.00%.

@@            Coverage Diff             @@
##             main    #1086      +/-   ##
==========================================
- Coverage   88.44%   88.28%   -0.16%     
==========================================
  Files         706      706              
  Lines       16342    16397      +55     
  Branches     2640     2652      +12     
==========================================
+ Hits        14453    14476      +23     
- Misses       1882     1914      +32     
  Partials        7        7

Impacted Files	Coverage Δ
packages/core/src/modules/ledger/LedgerApi.ts	`97.61% <ø> (ø)`
...e/src/modules/ledger/services/IndyLedgerService.ts	`53.84% <57.14%> (-16.69%)`	⬇️
...modules/dids/methods/sov/IndySdkSovDidRegistrar.ts	`94.20% <86.66%> (ø)`
.../modules/dids/methods/sov/IndySdkSovDidResolver.ts	`88.09% <88.09%> (ø)`
...ore/src/modules/ledger/services/IndyPoolService.ts	`96.05% <90.00%> (-3.95%)`	⬇️
packages/core/src/modules/dids/DidsModuleConfig.ts	`100.00% <100.00%> (ø)`
...ackages/core/src/modules/dids/methods/sov/index.ts	`100.00% <100.00%> (ø)`
packages/core/src/agent/MessageReceiver.ts	`81.98% <0.00%> (-2.71%)`	⬇️
packages/core/src/wallet/IndyWallet.ts	`55.43% <0.00%> (-1.13%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

…ver/registrar Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

TimoGlastra · 2022-11-07T09:02:16Z

Will put in a thorough review soon, but this is awesome! Thanks @sairanjit!

TimoGlastra

Thanks for the PR @sairanjit, left some comments

TimoGlastra · 2022-11-08T07:42:50Z

packages/core/src/modules/dids/methods/sov/IndySdkSovDidRegistrar.ts

+    alias: string,
+    role?: Indy.NymRole
+  ) {
+    const pool = this.indyPoolService.ledgerWritePool


This takes the pool from the ledgerWritePool, however you also added a config option to set the indyNamespace. I think we should get the pool based on the indyNamespace from the indyPoolService and use that as the pool to write to the ledger instead.

TimoGlastra · 2022-11-08T07:43:25Z

packages/core/src/modules/dids/methods/sov/IndySdkSovDidRegistrar.ts

+    did: string,
+    endpoints: IndyEndpointAttrib
+  ): Promise<void> {
+    const pool = this.indyPoolService.ledgerWritePool


Same here, should use pool based on the indyNamespace variable. I think for both methods we can pass the pool as an argument to this method.

TimoGlastra · 2022-11-08T07:44:05Z

packages/core/src/modules/dids/methods/sov/IndySdkSovDidRegistrar.ts

@@ -178,6 +240,7 @@ export interface SovDidCreateOptions extends DidCreateOptions {
  // As did:sov is so limited, we require everything needed to construct the did document to be passed
  // through the options object. Once we support did:indy we can allow the didDocument property.
  didDocument?: never
+  indyNamespace?: string


The indyNamespace property should be added to the options object

TimoGlastra · 2022-11-08T07:44:39Z

packages/core/src/modules/dids/methods/sov/IndySdkSovDidRegistrar.ts

@@ -102,7 +98,8 @@ export class SovDidRegistrar implements DidRegistrar {
      // Build did document.
      const didDocument = didDocumentBuilder.build()

-      const didIndyNamespace = this.indyPoolService.ledgerWritePool.config.indyNamespace
+      const pool = this.indyPoolService.getPoolForNamespace(options.indyNamespace)


We can pass this pool to the registerPublicDid and setEndpointsForDid methods

TimoGlastra · 2022-11-08T07:45:40Z

packages/core/src/modules/dids/methods/sov/IndySdkSovDidResolver.ts

+    return didResponse
+  }
+
+  public async getEndpointsForDid(agentContext: AgentContext, did: string) {


Suggested change

public async getEndpointsForDid(agentContext: AgentContext, did: string) {

private async getEndpointsForDid(agentContext: AgentContext, did: string) {

TimoGlastra · 2022-11-08T07:48:05Z

packages/core/src/modules/dids/methods/sov/__tests__/IndySdkSovDidResolver.test.ts

+      indySdkSovDidResolver = new IndySdkSovDidResolver(
+        indyPoolServiceMock,
+        agentConfig.agentDependencies,
+        new LoggerMock()


agentConfig.logger

TimoGlastra · 2022-11-08T07:48:26Z

packages/core/src/modules/ledger/LedgerApi.ts

@@ -55,6 +55,9 @@ export class LedgerApi {
    await this.ledgerService.connectToPools()
  }

+  /**
+   * @deprecated use agent.dids.create instead


TimoGlastra · 2022-11-08T07:51:50Z

packages/core/src/modules/ledger/services/IndyLedgerService.ts

@@ -131,6 +140,9 @@ export class IndyLedgerService {
    }
  }

+  /**
+   * @deprecated
+   */
  public async getEndpointsForDid(agentContext: AgentContext, did: string) {


I see you moved some methods to the indy pool service. Can we update all methods in the indy ledger service to call those new methods instead? As they're private in this class we can migrate them (instead of duplicate) without any issues

TimoGlastra · 2022-11-08T07:52:49Z

packages/core/src/modules/ledger/services/IndyLedgerService.ts

@@ -59,6 +59,9 @@ export class IndyLedgerService {
    return this.indyPoolService.connectToPools()
  }

+  /**
+   * @deprecated


Can you also mark getDidIndyWriteNamespace as deprecated?

TimoGlastra · 2022-11-08T07:53:27Z

packages/core/src/modules/ledger/services/IndyPoolService.ts

@@ -14,6 +14,7 @@ import { FileSystem } from '../../../storage/FileSystem'
 import { isSelfCertifiedDid } from '../../../utils/did'
 import { isIndyError } from '../../../utils/indyError'
 import { allSettled, onlyFulfilled, onlyRejected } from '../../../utils/promises'
+import { assertIndyWallet } from '../../../wallet/util/assertIndyWallet'
 import { IndyPool } from '../IndyPool'
 import { LedgerError } from '../error/LedgerError'
 import { LedgerNotConfiguredError } from '../error/LedgerNotConfiguredError'


Can you also mark get ledgerWritePool as deprecated?

…gerService Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

…sairanjit/aries-framework-javascript into feat/getPoolForNamespace-method

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

TimoGlastra · 2022-11-11T10:14:39Z

packages/core/src/modules/ledger/__tests__/IndyLedgerService.test.ts

+      jest
+        .spyOn(poolService, 'submitWriteRequest')
+        .mockRejectedValue(
+          new Error(
+            'Unable to satisfy matching TAA with mechanism "accept" and version "1.0" in pool.\n Found ["accept"] and version 2.0 in pool.'
+          )
+        )


We're not actually testing anymore that it throws an error if the config version does not match, we're just throwing a mocked error now. Can we move these tests to the pool service tests? (As that's where the logic is moved to right?)

So, Can I keep the same tests in both IndyLedgerService.test.ts and IndyPoolService.test.ts ?

What do you mean exactly? What we're testing here doesn't really test anything, but I do think we should have this test in the indy pool service. Doest hat answer your question?

Okay, I will update the test to catch the actual error from the function instead of mocking it and move the tests from IndyLedgerService.test.ts to IndyPoolService.test.ts. Is It right now?

After moving the tests to IndyPoolService.test.ts there are no tests left in the IndyLedgerService.test.ts file. So, Can I delete the IndyLedgerService.test.ts file?

Yes if there's no tests left that's fine 👍

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

…sairanjit/aries-framework-javascript into feat/getPoolForNamespace-method

TimoGlastra

👍

…ver/registrar (openwallet-foundation#1086) Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

feat: add getPoolForNamespace in indy pool service

81d1b5a

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

sairanjit requested a review from a team as a code owner November 6, 2022 07:54

sairanjit added 2 commits November 6, 2022 15:25

refactor: map indyNamespaces from pools

2334df9

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

refactor: remove dependency on indy ledger service from sov did resol…

e0e4b3c

…ver/registrar Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

sairanjit changed the title ~~feat: add getPoolForNamespace method in IndyPoolService~~ refactor: remove dependency on indy ledger service from sov did resolver/registrar Nov 6, 2022

sairanjit added 2 commits November 6, 2022 18:27

refactor: add indyNamespace as a argument to getPoolForNamespace method

d402eeb

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

ci: retrigger ci

31b7505

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

Merge branch 'main' into feat/getPoolForNamespace-method

520e944

TimoGlastra requested changes Nov 8, 2022

View reviewed changes

sairanjit added 2 commits November 10, 2022 01:27

refactor: remove logger mocks and remove duplicate methods in indyLed…

74497eb

…gerService Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

Merge branch 'feat/getPoolForNamespace-method' of https://github.com/…

e167bdc

…sairanjit/aries-framework-javascript into feat/getPoolForNamespace-method

TimoGlastra linked an issue Nov 10, 2022 that may be closed by this pull request

Allow to pass indyNamespace to SovDidRegistrar create method #1067

Closed

sairanjit and others added 2 commits November 10, 2022 23:45

fix: indyLedgerService tests

3b8d86c

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

Merge branch 'main' into feat/getPoolForNamespace-method

f02f10d

sairanjit requested a review from TimoGlastra November 11, 2022 04:08

Merge branch 'main' into feat/getPoolForNamespace-method

b2a2edc

TimoGlastra reviewed Nov 11, 2022

View reviewed changes

sairanjit and others added 3 commits November 11, 2022 19:45

Merge branch 'main' into feat/getPoolForNamespace-method

508f6d7

refactor: add tests from indyLedgerService to indyPoolService

df92305

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

Merge branch 'feat/getPoolForNamespace-method' of https://github.com/…

224f98d

…sairanjit/aries-framework-javascript into feat/getPoolForNamespace-method

TimoGlastra approved these changes Nov 11, 2022

View reviewed changes

TimoGlastra merged commit 85d62ec into openwallet-foundation:main Nov 11, 2022

berendsliedrecht mentioned this pull request Jan 18, 2023

fix/transport connection id animo/aries-framework-javascript#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove dependency on indy ledger service from sov did resolver/registrar #1086

refactor: remove dependency on indy ledger service from sov did resolver/registrar #1086

sairanjit commented Nov 6, 2022 •

edited

Loading

codecov-commenter commented Nov 6, 2022 •

edited

Loading

TimoGlastra commented Nov 7, 2022

TimoGlastra left a comment

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 8, 2022

TimoGlastra Nov 11, 2022

sairanjit Nov 11, 2022

TimoGlastra Nov 11, 2022

sairanjit Nov 11, 2022

sairanjit Nov 11, 2022

TimoGlastra Nov 11, 2022

TimoGlastra left a comment

	public async getEndpointsForDid(agentContext: AgentContext, did: string) {
	private async getEndpointsForDid(agentContext: AgentContext, did: string) {

refactor: remove dependency on indy ledger service from sov did resolver/registrar #1086

refactor: remove dependency on indy ledger service from sov did resolver/registrar #1086

Conversation

sairanjit commented Nov 6, 2022 • edited Loading

codecov-commenter commented Nov 6, 2022 • edited Loading

Codecov Report

TimoGlastra commented Nov 7, 2022

TimoGlastra left a 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

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

TimoGlastra left a comment

Choose a reason for hiding this comment

sairanjit commented Nov 6, 2022 •

edited

Loading

codecov-commenter commented Nov 6, 2022 •

edited

Loading