feat: break out indy wallet, better indy handling #396

TimoGlastra · 2021-07-20T07:59:48Z

Extracts part of the code in #360.

Main reasons for this are:

remove dependency on indy-sdk where it is not strictly required (mainly wallet interface is less indy-specific now)
make indy stuff more indy specific
other points below

I want to make a browser compatible wallet (already have a working one locally) so we can use the agent in the browser

Break out indy wallet

Fixes #330

It didn't make a lot of sense to me that the indy wallet was handling indy storage and ledger stuff. I think it belongs more in the ledger and storage services. This is also more in line with where we're headed with the shared components libraries

Rename LedgerService to IndyLedgerService. It's very indy focussed now.
move storage related methods to IndyStorageService
move ledger related methods to IndyLedgerService

Better indy message handling

Sometimes you would get very cryptic errors such as CommonInvalidParam without a stack trace or anything that would help you get a sense of what's erroring out. I've wrapped all indy calls with a try catch and added a new IndySdkError. This will take the indy error as input, but with very nice stack tracing so you know exactly where things went wrong, without losing information from the indy error

Signed-off-by: Timo Glastra <timo@animo.id>

codecov-commenter · 2021-07-20T08:11:55Z

Codecov Report

Merging #396 (9578e11) into main (5f2d512) will decrease coverage by 0.33%.
The diff coverage is 73.86%.

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   86.64%   86.30%   -0.34%     
==========================================
  Files         248      249       +1     
  Lines        5210     5256      +46     
  Branches      830      784      -46     
==========================================
+ Hits         4514     4536      +22     
- Misses        696      720      +24

Impacted Files	Coverage Δ
packages/core/src/agent/AgentConfig.ts	`90.90% <ø> (-0.32%)`	⬇️
...ges/core/src/agent/models/InboundMessageContext.ts	`91.66% <ø> (ø)`
packages/core/src/error/BaseError.ts	`50.00% <ø> (ø)`
.../core/src/modules/connections/ConnectionsModule.ts	`74.02% <0.00%> (ø)`
...modules/connections/repository/ConnectionRecord.ts	`98.36% <ø> (ø)`
...ckages/core/src/modules/routing/RecipientModule.ts	`64.00% <0.00%> (ø)`
.../src/modules/routing/repository/MediationRecord.ts	`91.17% <ø> (ø)`
packages/core/src/types.ts	`100.00% <ø> (ø)`
packages/core/src/utils/indyError.ts	`100.00% <ø> (+7.14%)`	⬆️
packages/core/src/error/IndySdkError.ts	`40.00% <40.00%> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f2d512...9578e11. Read the comment docs.

Signed-off-by: Timo Glastra <timo@animo.id>

burdettadam · 2021-07-20T17:07:07Z

packages/core/src/modules/routing/services/MessagePickupService.ts

@@ -16,14 +17,6 @@ export class MessagePickupService {
    this.messageRepository = messageRepository
  }

-  public async batchPickup(inboundConnection: InboundConnection) {


Was the batchPickup message removed intentionally?

It wasn't used because the RecipientModule directly creates a batch pickup message. I choose to remove this method, but it's probably better to use the service method instead. Will update

burdettadam · 2021-07-20T17:20:50Z

packages/core/src/storage/IndyStorageService.ts

  public async getAll(recordClass: BaseRecordConstructor<T>): Promise<T[]> {
-    const recordIterator = await this.wallet.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)
+    const recordIterator = this.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)


Is the search not async any more? Are we missing an await?

Suggested change

const recordIterator = this.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)

const recordIterator = await this.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)

this.search returns a generator, which we can use as an async iterator. So the this.search call itself is not async

berendsliedrecht

Some minor things. Nothing about the functionality but more I think some overlooked things.

Great changes! Love the errors :-)

packages/core/src/agent/Agent.ts

packages/core/src/agent/EnvelopeService.ts

packages/core/src/agent/__tests__/Agent.test.ts

packages/core/src/modules/connections/__tests__/ConnectionService.test.ts

berendsliedrecht · 2021-07-28T07:34:39Z

packages/core/src/wallet/IndyWallet.ts

-        await this.create(walletConfig, walletCredentials)
-        await this.open(walletConfig, walletCredentials)
+        await this.create(walletConfig)
+        await this.open(walletConfig)
      } else {
        throw error


Should this not be an AriesFrameworkError or IndySdkError?

this.open already handles this, so we'll always get an aries framework error here

packages/core/src/wallet/IndyWallet.ts

berendsliedrecht · 2021-07-28T07:39:55Z

packages/core/src/wallet/IndyWallet.ts

+  public async verify(signerVerkey: string, data: Buffer, signature: Buffer): Promise<boolean> {
+    try {
+      // check signature
+      const isValid = await this.indy.cryptoVerify(signerVerkey, data, signature)


I think in this case isValid adds some useful context to what cryptoVerify (and verify()) returns

packages/core/src/decorators/signature/SignatureDecoratorUtils.test.ts

…indy

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra · 2021-08-02T15:37:30Z

@jakubkoci could you please take a look at this PR? The slow review process is really slowing me down

packages/core/src/types.ts

packages/core/src/wallet/IndyWallet.ts

packages/core/src/modules/indy/services/IndyHolderService.ts

jakubkoci · 2021-08-03T13:41:11Z

packages/core/src/error/IndySdkError.ts

+
+import { AriesFrameworkError } from './AriesFrameworkError'
+
+export class IndySdkError extends AriesFrameworkError {


This is kind of interesting. I would assume that IndySdk is lower-level detail than framework level error. Therefore, I'm not sure about IndySdkError extending AriesFrameworkError

The normal IndyError is lower-level detail than the framework level error. However, because the normal error is not very nice in stacktracing/information we've wrapped it with a framework specific error.

To me AriesFrameworkError is the basis for all framework created errors (which is the case if we wrap the original error). I think it can be beneficial to have a single base error that allows to catch all framework errors. Maybe AriesFrameworkError is not the best name as the base error

…indy

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra added 6 commits July 20, 2021 00:58

feat: break out indy wallet, better indy handling

fddcd0e

Signed-off-by: Timo Glastra <timo@animo.id>

refactor(core): remove indy specific wallet stuff

20d3b3a

Signed-off-by: Timo Glastra <timo@animo.id>

fix(core): camelCase for unpack properties

fee72a4

Signed-off-by: Timo Glastra <timo@animo.id>

build(core): fix node resolution

b2be40a

Signed-off-by: Timo Glastra <timo@animo.id>

chore: update samples

303e571

Signed-off-by: Timo Glastra <timo@animo.id>

fix(core): remove duplicate is optional decorator

1d55fe2

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested a review from a team as a code owner July 20, 2021 07:59

TimoGlastra mentioned this pull request Jul 20, 2021

refactor: only use ready connection for inbound #360

Merged

chore: remove unused dependencies from yarn.lock

7d43354

Signed-off-by: Timo Glastra <timo@animo.id>

test: add missing indy assignment

1082ce0

Signed-off-by: Timo Glastra <timo@animo.id>

burdettadam approved these changes Jul 20, 2021

View reviewed changes

TimoGlastra requested review from JamesKEbert and berendsliedrecht July 27, 2021 15:04

berendsliedrecht requested changes Jul 28, 2021

View reviewed changes

TimoGlastra added 4 commits July 28, 2021 21:12

Merge remote-tracking branch 'upstream/main' into refactor/break-out-…

531c8b0

…indy

refactor(core): remove unecessary indy-sdk imports

918cb5b

Signed-off-by: Timo Glastra <timo@animo.id>

refactor(core): PR feedback

056ae3f

Signed-off-by: Timo Glastra <timo@animo.id>

refactor(core): remove unecessary indy-sdk imports

2fa2c20

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested a review from berendsliedrecht July 28, 2021 19:37

docs: remove mediator

79245e3

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht approved these changes Jul 29, 2021

View reviewed changes

chore: @types/node resolutions

9578e11

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested a review from jakubkoci July 29, 2021 21:21

jakubkoci reviewed Aug 3, 2021

View reviewed changes

TimoGlastra added 3 commits August 4, 2021 11:25

Merge remote-tracking branch 'upstream/main' into refactor/break-out-…

6600f96

…indy

docs: update docs for monorepo

697b23d

Signed-off-by: Timo Glastra <timo@animo.id>

refactor: pr feedback

229f178

Signed-off-by: Timo Glastra <timo@animo.id>

jakubkoci approved these changes Aug 4, 2021

View reviewed changes

TimoGlastra merged commit 9f1a4a7 into openwallet-foundation:main Aug 4, 2021

TimoGlastra deleted the refactor/break-out-indy branch August 4, 2021 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: break out indy wallet, better indy handling #396

feat: break out indy wallet, better indy handling #396

TimoGlastra commented Jul 20, 2021

codecov-commenter commented Jul 20, 2021 •

edited

Loading

burdettadam Jul 20, 2021

TimoGlastra Jul 21, 2021 •

edited

Loading

burdettadam Jul 20, 2021

TimoGlastra Jul 21, 2021

berendsliedrecht left a comment

berendsliedrecht Jul 28, 2021

TimoGlastra Jul 28, 2021

berendsliedrecht Jul 28, 2021

TimoGlastra Jul 28, 2021

TimoGlastra commented Aug 2, 2021

jakubkoci Aug 3, 2021

TimoGlastra Aug 4, 2021

	const recordIterator = this.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)
	const recordIterator = await this.search(recordClass.type, {}, IndyStorageService.DEFAULT_QUERY_OPTIONS)


		import { AriesFrameworkError } from './AriesFrameworkError'

		export class IndySdkError extends AriesFrameworkError {

feat: break out indy wallet, better indy handling #396

feat: break out indy wallet, better indy handling #396

Conversation

TimoGlastra commented Jul 20, 2021

Break out indy wallet

Better indy message handling

codecov-commenter commented Jul 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

TimoGlastra Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendsliedrecht 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

TimoGlastra commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 20, 2021 •

edited

Loading

TimoGlastra Jul 21, 2021 •

edited

Loading