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(proofs): await shouldAutoRespond to correctly handle the check #1116

Merged

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Nov 23, 2022

We did not await the shouldAutoRespondTo... and if we then check if the value is truthy, it is because it is a Promise and not the contained value of boolean.

Abstracting the async function call to a variable above makes an IDE error on the if statement is it will always be truthy if not-awaited.

@TimoGlastra Would it make sense to add a rule that you must mark void/await for async calls? that would've probably avoided this, quite big, bug.

closes #1095

@berendsliedrecht berendsliedrecht requested a review from a team as a code owner November 23, 2022 13:53
@berendsliedrecht berendsliedrecht changed the title fix(proof): removed fix(proof): await shouldAutoRespond to correctly handle the check Nov 23, 2022
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

🔥
Just a little edit on the scope for changelog ("proofs" instead of "proof")

@genaris genaris changed the title fix(proof): await shouldAutoRespond to correctly handle the check fix(proofs): await shouldAutoRespond to correctly handle the check Nov 23, 2022
@berendsliedrecht
Copy link
Contributor Author

@TimoGlastra Would it be possible to get this in 0.3.0? I assume if it goes to main now it will be in 0.3.0, right?

@genaris
Copy link
Contributor

genaris commented Nov 23, 2022

@TimoGlastra Would it be possible to get this in 0.3.0? I assume if it goes to main now it will be in 0.3.0, right?

We are working on 0.3.0.alpha.xxx, so every commit will be included to the 0.3.0 release. A new PR will be created later on (I think Timo created the current release PR just to help me on the migration guide, as it generates the changelog).

@berendsliedrecht berendsliedrecht enabled auto-merge (squash) November 23, 2022 17:09
@TimoGlastra
Copy link
Contributor

@TimoGlastra Would it make sense to add a rule that you must mark void/await for async calls? that would've probably avoided this, quite big, bug.

Yes I think we should add no-floating-promises and no-misused-promises

@codecov-commenter
Copy link

Codecov Report

Merging #1116 (3841a15) into main (03cdf39) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1116      +/-   ##
==========================================
- Coverage   88.28%   88.16%   -0.13%     
==========================================
  Files         705      705              
  Lines       16399    16405       +6     
  Branches     2657     2657              
==========================================
- Hits        14478    14463      -15     
- Misses       1914     1935      +21     
  Partials        7        7              
Impacted Files Coverage Δ
...les/credentials/protocol/v1/V1CredentialService.ts 93.43% <ø> (ø)
...ore/src/modules/proofs/ProofResponseCoordinator.ts 100.00% <100.00%> (ø)
...oofs/protocol/v1/handlers/V1PresentationHandler.ts 96.15% <100.00%> (+0.15%) ⬆️
...otocol/v1/handlers/V1ProposePresentationHandler.ts 85.00% <100.00%> (+0.38%) ⬆️
...otocol/v1/handlers/V1RequestPresentationHandler.ts 88.37% <100.00%> (+0.27%) ⬆️
...oofs/protocol/v2/handlers/V2PresentationHandler.ts 96.15% <100.00%> (+0.15%) ⬆️
...otocol/v2/handlers/V2ProposePresentationHandler.ts 81.81% <100.00%> (+0.56%) ⬆️
...otocol/v2/handlers/V2RequestPresentationHandler.ts 97.05% <100.00%> (+0.08%) ⬆️
...e/src/modules/proofs/protocol/v1/V1ProofService.ts 74.87% <0.00%> (-5.39%) ⬇️

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

@berendsliedrecht
Copy link
Contributor Author

Apparently the tests were written for the incorrect way of doing autoAccept. Will fix this ASAP tomorrow.

Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
@berendsliedrecht
Copy link
Contributor Author

Sorry that it took so long, the bug was a bit bigger than just a missing "await". Our deep equality check did not work, and I had to change it. (also added some more tests to be sure).

Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
@berendsliedrecht berendsliedrecht merged commit f294129 into openwallet-foundation:main Dec 1, 2022
@berendsliedrecht berendsliedrecht deleted the fix/auto-respond branch December 1, 2022 15:15
@berendsliedrecht
Copy link
Contributor Author

I forgot auto merge was enabled. @TimoGlastra @genaris if you see something wrong, I will revert the pr.

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.

AutoAcceptProof.ContentApproved on latest AFJ works incorrectly
4 participants