refactor: only use ready connection for inbound #360

TimoGlastra · 2021-07-03T22:30:09Z

Breaking up this PR as discussed. See #396 for first sub-PR

This PR became a bit bigger than anticipated, but one change led to another and so on. However I do think this PR packs some nice improvements.

validation on incoming messages

Fixes #68

Although used a lot, we weren't putting the validation decorators to good use yet (@IsString, etc...). I've enabled them and made updates where needed to make them valid. Already found some bugs. I think once we update the AFJ backchannel for AATH we'll probably find some more interop issues with ACA-Py.

Only attach connection to inbound message context if sender and receiver match and connection is ready

Fixes #76

We were attaching the connection before it was ready and didn't do all necessary checks to make sure we're actually in the context of this connection. The connection is now only attached if the connection is ready to be used and fully matches with the incoming recipient and sender key.

~~### 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

Also fixed a bug where the mediator would terminate on unhandled promise

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

codecov-commenter · 2021-07-03T22:37:57Z

Codecov Report

Merging #360 (075cca9) into main (52c7897) will increase coverage by 0.00%.
The diff coverage is 84.43%.

@@           Coverage Diff           @@
##             main     #360   +/-   ##
=======================================
  Coverage   85.65%   85.66%           
=======================================
  Files         253      254    +1     
  Lines        5467     5498   +31     
  Branches      878      884    +6     
=======================================
+ Hits         4683     4710   +27     
- Misses        783      787    +4     
  Partials        1        1

Impacted Files	Coverage Δ
packages/core/src/modules/proofs/ProofsModule.ts	`90.00% <ø> (ø)`
.../core/src/modules/routing/handlers/BatchHandler.ts	`50.00% <0.00%> (ø)`
...src/modules/routing/handlers/BatchPickupHandler.ts	`54.54% <0.00%> (-5.46%)`	⬇️
.../core/src/modules/routing/messages/BatchMessage.ts	`56.52% <55.55%> (+4.14%)`	⬆️
packages/core/src/agent/Dispatcher.ts	`81.39% <62.50%> (-5.10%)`	⬇️
...ecorators/transport/TransportDecoratorExtension.ts	`78.94% <66.66%> (-4.39%)`	⬇️
packages/core/src/agent/MessageReceiver.ts	`89.18% <69.23%> (-3.24%)`	⬇️
...c/modules/routing/handlers/KeylistUpdateHandler.ts	`92.30% <75.00%> (+0.64%)`	⬆️
.../modules/connections/services/ConnectionService.ts	`95.62% <82.35%> (-1.55%)`	⬇️
.../modules/credentials/messages/CredentialPreview.ts	`85.71% <83.33%> (+1.09%)`	⬆️
... and 45 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 52c7897...075cca9. Read the comment docs.

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

JamesKEbert · 2021-07-17T07:03:12Z

These are some good additions--and I will give some additional review once you find this is in a good state @TimoGlastra as discussed in the WG call on the 15th, which IIRC may include you splitting this out into additional PRs.

…-connection

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

TimoGlastra · 2021-09-19T15:54:12Z

@jakubkoci @JamesKEbert @blu3beri I've updated this PR with latest changes. Although there are still a lot of changed files, most of it is adding class-validator decorators.

Could you take a look so we can get it merged? :)

berendsliedrecht

Very nice! love the addition of a lot more validators.

Just left some minor points, but nothing major.

berendsliedrecht · 2021-09-20T07:15:07Z

packages/core/src/agent/MessageReceiver.ts

-      // We check whether the sender key is the same as the key we have stored in the connection
-      // otherwise everyone could send messages to our key and we would just accept
-      // it as if it was send by the key of the connection.
+      // Throw error if the recipient key (ourKey) does not match the key of the connection record
      if (connection && connection.theirKey != null && connection.theirKey != senderKey) {


Not your line, but why do we check if connection.theirKey is not null? The last check connection.theirKey != senderKey should suffice.

Also we should do !==

It doesn't completely suffice, as in this case if connection.theirKey is null, it will not throw the error. If theirKey is null, and senderKey isn't (it will never be null due to check above) it will throw the error.

if connection.theirKey is null it means the connection does not have their key yet (we've sent an invitation, but didn't receive a request yet). This check is mostly to check if a connection that already has theirKey matches the key of the message we received

Agree on the !== however

berendsliedrecht · 2021-09-20T07:26:41Z

packages/core/src/decorators/timing/TimingDecorator.ts

@@ -22,6 +22,7 @@ export class TimingDecorator {
  @Expose({ name: 'in_time' })
  @Type(() => Date)
  @IsDate()
+  @IsOptional()


Not really a comment, but is there a particular order we want to add these decorators?

Nope. I'm not sure if there are sorters for decorators (as the order can matter I think) and doing it manually seems tedious

berendsliedrecht · 2021-09-20T07:28:36Z

packages/core/src/decorators/transport/TransportDecorator.test.ts

-  validateOrReject(JsonTransformer.fromJSON(obj, TransportDecorator))
-const expectValid = (obj: Record<string, unknown>) => expect(validTranport(obj)).resolves.toBeUndefined()
-const expectInvalid = (obj: Record<string, unknown>) => expect(validTranport(obj)).rejects.not.toBeNull()
+const validTransport = (obj: Record<string, unknown>) =>


Nitpick, but could we make obj a bit more verbose?

berendsliedrecht · 2021-09-20T07:38:01Z

packages/core/src/modules/credentials/models/Credential.ts

@@ -19,11 +19,13 @@ export class Credential {
  @Expose({ name: 'cred_info' })
  @Type(() => IndyCredentialInfo)
  @ValidateNested()
+  @IsInstance(IndyCredentialInfo)


There is a common pattern of adding the @type decorator and the @IsInstance decorator. Is there a way to combine these decorators? Can be in a seperate PR.

Could be. I'm not sure if it is easy to combine two decorators and make the work like before (as it needs the field data)

berendsliedrecht · 2021-09-20T07:41:13Z

packages/core/src/modules/proofs/ProofsModule.ts

@@ -141,7 +142,7 @@ export class ProofsModule {
   */
  public async requestProof(
    connectionId: string,
-    proofRequestOptions: ProofRequestOptions,
+    proofRequestOptions: RequestProofOptions,


Why the reorder of words? Shouldn't we also rename the param then? And the line below:

config?: ProofRequestConfig to config?: RequestProofConfig or requestProofConfig?: RequestProofConfig

Yeah this is kinda messy. I'll take a look

berendsliedrecht · 2021-09-20T08:11:31Z

packages/core/src/modules/routing/messages/KeylistUpdateMessage.ts


 import { AgentMessage } from '../../../agent/AgentMessage'

+export enum KeylistUpdateAction {
+  add = 'add',


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

TimoGlastra · 2021-09-29T19:23:09Z

If no one else is going to review, I'm going to merge this PR

…-connection

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

TimoGlastra added 8 commits July 3, 2021 19:50

refactor: only use ready connection for inbound

90bbc63

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

feat: validate messages before processing

6d1fb59

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

feat: improve message validation decorators

3832491

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

feat: add @isInstance validation

a20b444

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

refactor: use map for better class validation

3db7c19

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

feat: break out indy wallet, better indy handling

cb52bb4

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

refactor: use internal message validator

5d75161

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

style: eslint warnings

aa53d54

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

TimoGlastra requested a review from a team as a code owner July 3, 2021 22:30

fix: remove unnecessary validation

485fff0

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

TimoGlastra mentioned this pull request Jul 20, 2021

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

Merged

janrtvld mentioned this pull request Sep 14, 2021

[rest] OpenAPI validations for nested objects openwallet-foundation/credo-ts-ext#17

Closed

TimoGlastra added 5 commits September 19, 2021 16:44

Merge remote-tracking branch 'upstream/main' into refactor/only-ready…

941e456

…-connection

style: eslint warnings

c4c7e82

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

test(core): fix incorrect base64 encoding

0c57eeb

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

refactor(core): remove unecessary elif statement

707c7aa

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

fix(core): take into account return routing

689ad29

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

TimoGlastra requested review from JamesKEbert, jakubkoci and berendsliedrecht September 19, 2021 15:54

berendsliedrecht approved these changes Sep 20, 2021

View reviewed changes

fix(core): resolved feedback

075cca9

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

TimoGlastra added 2 commits October 2, 2021 09:07

Merge remote-tracking branch 'upstream/main' into refactor/only-ready…

5708022

…-connection

test: fix connection tests after merge

436bb5c

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

TimoGlastra merged commit cb256ca into openwallet-foundation:main Oct 2, 2021

TimoGlastra deleted the refactor/only-ready-connection branch October 28, 2021 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: only use ready connection for inbound #360

refactor: only use ready connection for inbound #360

TimoGlastra commented Jul 3, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading

JamesKEbert commented Jul 17, 2021

TimoGlastra commented Sep 19, 2021

berendsliedrecht left a comment

berendsliedrecht Sep 20, 2021

TimoGlastra Sep 20, 2021

TimoGlastra Sep 20, 2021

berendsliedrecht Sep 20, 2021

TimoGlastra Sep 20, 2021

berendsliedrecht Sep 20, 2021

berendsliedrecht Sep 20, 2021

TimoGlastra Sep 20, 2021

berendsliedrecht Sep 20, 2021

TimoGlastra Sep 20, 2021

berendsliedrecht Sep 20, 2021

TimoGlastra commented Sep 29, 2021

refactor: only use ready connection for inbound #360

refactor: only use ready connection for inbound #360

Conversation

TimoGlastra commented Jul 3, 2021 • edited Loading

validation on incoming messages

Only attach connection to inbound message context if sender and receiver match and connection is ready

Better indy message handling

codecov-commenter commented Jul 3, 2021 • edited Loading

Codecov Report

JamesKEbert commented Jul 17, 2021

TimoGlastra commented Sep 19, 2021

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

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 commented Sep 29, 2021

TimoGlastra commented Jul 3, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading