Skip to content

fix(crypto): run ajv validator again when encountering exceptions#3008

Merged
faustbrian merged 1 commit intodevelopfrom
fix/crypto/schema-exceptions
Oct 2, 2019
Merged

fix(crypto): run ajv validator again when encountering exceptions#3008
faustbrian merged 1 commit intodevelopfrom
fix/crypto/schema-exceptions

Conversation

@spkjp
Copy link
Copy Markdown
Contributor

@spkjp spkjp commented Oct 2, 2019

Summary

If the block validation fails and there are any exceptions, instead of continuing right away, rerun the schema validation without bailing out on the first encountered error. This ensures all bignumber
properties are properly converted.

Fixes #3003

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

If the block validation fails and there are any exceptions, instead of
continuing right away, rerun the schema validation without bailing out
on the first encountered error. This ensures all bignumber properties
are properly converted.
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 2, 2019

Codecov Report

Merging #3008 into develop will increase coverage by 0.03%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3008      +/-   ##
===========================================
+ Coverage    66.06%   66.09%   +0.03%     
===========================================
  Files          420      420              
  Lines        10036    10039       +3     
  Branches       510      511       +1     
===========================================
+ Hits          6630     6635       +5     
+ Misses        3364     3363       -1     
+ Partials        42       41       -1
Impacted Files Coverage Δ
packages/crypto/src/validation/keywords.ts 91.52% <100%> (ø) ⬆️
packages/crypto/src/blocks/block.ts 89.21% <66.66%> (+1.33%) ⬆️
packages/crypto/src/transactions/verifier.ts 92% <0%> (+4%) ⬆️

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 14dfc07...869318c. Read the comment docs.

@faustbrian faustbrian merged commit 81a7cbc into develop Oct 2, 2019
@ghost ghost deleted the fix/crypto/schema-exceptions branch October 2, 2019 04:25
// Validate again without bailing out on the first error to ensure that all properties get properly converted if necessary
validator.validateException("block", data);
} else {
throw new BlockSchemaError(data.height, error);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can remove this, it is never going to be executed. If error is true and the long expression is false then we would have thrown already earlier.

if (A && !B) {
  throw E;
}
if (A) {
  if (B) {
    C;
  } else {
    throw E; // this will never be executed
  }
}

Or even better: remove the first if (lines 14-20) https://github.com/ArkEcosystem/core/pull/3008/files#diff-915b384fb6553933d473ac42c25419c2L14-L20 that code is equivalent to:

if (A) {
  if (B) {
    C;
  } else {
    throw E;
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, lines 14-20 were intended to be removed.

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.

3 participants