Introduce integer ReturnValueTest variant and use it to coerce blocktime value string in TimeCondition#764
Draft
cygnusv wants to merge 6 commits intonucypher:signing-epicfrom
Draft
Introduce integer ReturnValueTest variant and use it to coerce blocktime value string in TimeCondition#764cygnusv wants to merge 6 commits intonucypher:signing-epicfrom
cygnusv wants to merge 6 commits intonucypher:signing-epicfrom
Conversation
We will use it specifically for certain ReturnValueTypes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## signing-epic #764 +/- ##
===============================================
Coverage ? 90.20%
===============================================
Files ? 97
Lines ? 8584
Branches ? 306
===============================================
Hits ? 7743
Misses ? 838
Partials ? 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
Converted to draft after conversation I had with @cygnusv about a potential alternative strategy. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of PR:
Required reviews:
What this does:
High-level summary
We fixed backend errors when
returnValueTest.valuewas given as an integer string (e.g."1701428400") for conditions that compare against an integer (e.g. blocktime). Rather than coercing integer-shaped strings for all blockchain params (which would assume intent everywhere), we introduced a variant return-value-test schema that only coerces such strings where the comparison is known to be integer-based. TimeCondition now uses this variant, so string values like"1701428400"are accepted and serialized as numbers. The generic blockchain return-value-test schema is unchanged and does not coerce; only conditions that explicitly use the integer variant (currently TimeCondition) get string-to-number coercion forvalue.Relevant API changes
context:integerStringSchema,integerBlockchainParamOrContextParamSchema.shared):blockchainIntegerReturnValueTestSchema,BlockchainIntegerReturnValueTestProps.returnValueTest.valuemay be an integer string (e.g."1701428400"); it is validated and serialized (e.g. viatoObj()) as a number. Number and bigint inputs are unchanged.valueas number or bigint are unchanged. Code using the genericblockchainReturnValueTestSchemaand passing integer strings will now see those values remain strings in the parsed result (previously they were coerced globally; that coercion is now limited to the integer variant).Issues fixed/closed:
Fixes issue seen by an adopter after integrating TACo and accidentally passing a string integer to a
TimeCondition.taco-webdidn't complain and only the nodes did, which returned with the following error:Why it's needed:
Notes for reviewers: