Support prefix notation for compound conditions #226
Conversation
Codecov Report
@@ Coverage Diff @@
## tdec-epic #226 +/- ##
=============================================
+ Coverage 82.35% 83.70% +1.34%
=============================================
Files 37 37
Lines 958 945 -13
Branches 117 116 -1
=============================================
+ Hits 789 791 +2
+ Misses 162 147 -15
Partials 7 7
|
…ed classes are only convenience objects, but underlying they are still contract conditions etc.
…can be used for compound conditions. Update some tests.
derekpierre
force-pushed
the
prefix-conditions
branch
from
3212506
to
63c22f6
Compare
derekpierre
changed the title
derekpierre
requested review from
KPrasch,
cygnusv,
theref,
manumonti and
piotr-roslaniec
|
|
||
| export const STANDARD_CONTRACT_TYPES = ['ERC20', 'ERC721']; | ||
|
|
||
| const functionAbiVariable = Joi.object({ | ||
| internalType: Joi.string().required(), | ||
| internalType: Joi.string(), // TODO is this needed? |
There was a problem hiding this comment.
I think yes, working on a related change here: #228
|
|
||
| describe('accepts either standardContractType or functionAbi but not both or none', () => { | ||
| const standardContractType = 'ERC20'; | ||
| const functionAbi = { |
There was a problem hiding this comment.
There's a testFunctionAbi in testVariables.ts if we want to reuse it.
| it.each([ | ||
| erc721BalanceCondition, | ||
| contractConditionNoAbi, | ||
| contractConditionWithAbi, | ||
| rpcCondition, | ||
| timeCondition, | ||
| compoundCondition, | ||
| ])('serializes to and from json', async (condition) => { |
… for contract condition.
| // no "operator" nor "method" value | ||
| { | ||
| randoKey: 'randoValue', | ||
| otherKey: 'otherValue', | ||
| }, | ||
| // invalid "method" and no "contractAddress" | ||
| { | ||
| method: 'doWhatIWant', | ||
| returnValueTest: { | ||
| index: 0, | ||
| comparator: '>', | ||
| value: '100', | ||
| }, | ||
| chain: 5, | ||
| }, | ||
| // condition with wrong method "method" and no contract address | ||
| { | ||
| ...testTimeConditionObj, | ||
| method: 'doWhatIWant', | ||
| }, | ||
| // rpc condition (no contract address) with disallowed method | ||
| { | ||
| ...testRpcConditionObj, | ||
| method: 'isPolicyActive', | ||
| }, |
There was a problem hiding this comment.
What do you think about using permutations here to exhaustively test for every possible incorrect variant? Nothing from with the current implementation, I'm just interested in your thoughts.
There was a problem hiding this comment.
I'm intrigued - could you elaborate a bit more on what you are thinking? Just want to make sure I understand.
The goal for this test was to test the ConditionSet.fromObj(...) branching logic for invalid data.
There was a problem hiding this comment.
We could generate a set of all possible permutations of the form <field>: <value>, where both field and value may contain valid and invalid values. From this set we remove permutations we know to be correct. Others should fail the validation, and so the test cases would be successful.
I think without a property-based testing library this is tiresome to implement. I did consider this type of testing in other places on different occasions.
There was a problem hiding this comment.
This could be good in a variety of areas for hardening testing. I guess one thing, based on your comment, is effort-to-benefit ratio, but if there is a tool in typescript to help with this that could change the equation and be pretty useful.
Assuming we specify the field validation correctly 😅 ., the JOI schema definitions help us in this general area; but hardening of tests is a good thing.
In this case, it's less about the validation by the Joi schema (actual condition), and more about being able to figure out what potential type of condition it is and then having that specific condition type perform further validation. The logic for determining the type of condition is pretty specific, so randomizing field values may provide marginal benefit in this case, but could provide larger benefit in other areas of the code. Good thing to keep in mind moving forward 👍 .
Type of PR:
Required reviews:
What this does:
Implements prefix notation for compound conditions in
nucpyher-ts.Related to:
Issues fixed/closed:
Fixes #224 .
Why it's needed:
Notes for reviewers:
ConditionSetshould be renamed, and will be when we introduce versioning in a follow-up PR.