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

fix(oob): check service is string instance #814

Merged

Conversation

TimoGlastra
Copy link
Contributor

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

Adds a check whether the service is instanceof String as mentioned in #812. this is needed due to the class-validator logic sadly.

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested a review from a team as a code owner May 30, 2022 11:15
@@ -103,7 +103,7 @@ export class OutOfBandInvitation extends AgentMessage {
// TODO: this only takes into account inline didcomm services, won't work for public dids
public getRecipientKeys(): Key[] {
return this.services
.filter((s): s is OutOfBandDidCommService => typeof s !== 'string')
.filter((s): s is OutOfBandDidCommService => typeof s !== 'string' && !(s instanceof String))
.map((s) => s.recipientKeys)
Copy link
Contributor

@Iskander508 Iskander508 May 30, 2022

Choose a reason for hiding this comment

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

Just a comment. The error mentioned in #812 actually came from the ...curr, because the previous map returns [undefined] in case of public did is used in the invitation.

So I'd suggest to modify the second mapping something like this:

Suggested change
.map((s) => s.recipientKeys)
.map((s) => s.recipientKeys || [])

or

     .reduce((acc, curr = []) => [...acc, ...curr], [])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm the filter method should filter out all non-string values, so it shouldn't be possible there is a type undefined in the array. It think by adding this to the filter it won't have the undefined anymore.

Copy link
Contributor

@Iskander508 Iskander508 May 30, 2022

Choose a reason for hiding this comment

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

s.recipientKeys was undefined. The json parser converts the string did into some weird object (maybe it was indeed String sorry for confusion).

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2022

Codecov Report

Merging #814 (ffb8eb2) into main (478fda3) will increase coverage by 1.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #814      +/-   ##
==========================================
+ Coverage   86.53%   87.70%   +1.17%     
==========================================
  Files         475      438      -37     
  Lines       12615    10823    -1792     
  Branches     2298     1892     -406     
==========================================
- Hits        10916     9492    -1424     
+ Misses       1594     1269     -325     
+ Partials      105       62      -43     
Impacted Files Coverage Δ
...re/src/modules/oob/messages/OutOfBandInvitation.ts 95.00% <100.00%> (ø)
packages/core/src/agent/MessageSender.ts 84.53% <0.00%> (-2.06%) ⬇️
...ules/routing/services/MediationRecipientService.ts 83.13% <0.00%> (-1.24%) ⬇️
...c/modules/dids/domain/createPeerDidFromServices.ts 96.55% <0.00%> (-1.23%) ⬇️
.../modules/connections/services/ConnectionService.ts 86.80% <0.00%> (-1.13%) ⬇️
...e/src/modules/dids/methods/peer/peerDidNumAlgo2.ts 97.77% <0.00%> (-0.83%) ⬇️
packages/core/src/utils/type.ts 100.00% <0.00%> (ø)
packages/core/src/utils/index.ts 100.00% <0.00%> (ø)
packages/core/src/crypto/index.ts 100.00% <0.00%> (ø)
packages/core/src/crypto/JwsService.ts 90.90% <0.00%> (ø)
... and 46 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 478fda3...ffb8eb2. Read the comment docs.

@TimoGlastra TimoGlastra enabled auto-merge (squash) May 31, 2022 10:17
@TimoGlastra TimoGlastra merged commit bd1e677 into openwallet-foundation:main May 31, 2022
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.

4 participants