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 incorrect Condition parsing #239

Merged

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Jul 7, 2023

Type of PR:

  • Bugfix

Required reviews:

  • 2

What this does:

  • Fixes a bug where conditions would be incorrectly deserialized from their string format, leading to ConditionContext attempting to access undefined fields
  • Changes how ConditionContext is prepared for Porter to fix this issue passing silently in tests

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

  • Need to back-port this to alpha tree

Comment on lines -106 to +108
.map((condition) => JSON.parse(condition.toString()))
.reduce((acc: Record<string, string>[], val) => acc.concat(val), []);
.map((condition) => ConditionExpression.fromJSON(condition.toString()))
.reduce((acc: ConditionExpression[], val) => acc.concat(val), [])
.map((condExpr: ConditionExpression) => condExpr.condition);
Copy link
Contributor Author

@piotr-roslaniec piotr-roslaniec Jul 7, 2023

Choose a reason for hiding this comment

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

This out-of-date condition handling was the root cause

@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review July 7, 2023 13:31
@codecov-commenter
Copy link

Codecov Report

Merging #239 (298fe22) into beta (de6dfaf) will increase coverage by 0.18%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             beta     #239      +/-   ##
==========================================
+ Coverage   83.47%   83.66%   +0.18%     
==========================================
  Files          37       37              
  Lines         968      967       -1     
  Branches      121      121              
==========================================
+ Hits          808      809       +1     
+ Misses        154      152       -2     
  Partials        6        6              
Impacted Files Coverage Δ
src/characters/porter.ts 34.37% <0.00%> (+2.94%) ⬆️
src/characters/pre-recipient.ts 83.09% <87.50%> (-0.96%) ⬇️

Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

Thanks for fixing this so quickly.

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

@piotr-roslaniec piotr-roslaniec merged commit ae56829 into nucypher:beta Jul 8, 2023
@piotr-roslaniec piotr-roslaniec deleted the fix-undefined-context branch July 8, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

5 participants