Prefix notation for logical compound conditions #3140

derekpierre · 2023-06-09T20:39:24Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:
Replaces infix notation with prefix notation for logical combinations of conditions.

Issues fixed/closed:
Fixes #3058

Why it's needed:
This is needed to disambiguate logical groupings as outlined in #3058 .

For example:

{
   "operator":"or",
   "operands":[
      {
         "chain":131277322940537,
         "method":"ownerOf",
         "parameters":[
            5954
         ],
         "returnValueTest":{
            "value":":userAddress",
            "comparator":"=="
         },
         "contractAddress":"0xaDD9D957170dF6F33982001E4c22eCCdd5539118",
         "standardContractType":"ERC721"
      },
      {
         "chain":131277322940537,
         "method":"blocktime",
         "returnValueTest":{
            "value":0,
            "comparator":">"
         }
      },
      {
         "operator":"and",
         "operands":[
            {
               "chain":131277322940537,
               "method":"eth_getBalance",
               "parameters":[
                  ":userAddress"
               ],
               "returnValueTest":{
                  "value":1000000000000000000000000,
                  "comparator":"=="
               }
            },
            {
               "chain":131277322940537,
               "method":"balanceOf",
               "parameters":[
                  ":userAddress"
               ],
               "returnValueTest":{
                  "value":0,
                  "comparator":"=="
               },
               "contractAddress":"0xaDD9D957170dF6F33982001E4c22eCCdd5539118",
               "standardContractType":"ERC20"
            }
         ]
      }
   ]
}

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

This will have downstream effect on nucypher-ts, and how logical operators are used there - nucypher/taco-web#224

jMyles · 2023-06-12T18:19:57Z

Since the convo on #3058 stalled, I just want to confirm:

Are we OK with proceeding with a format which is not (easily) checkable on-chain?

derekpierre · 2023-06-12T19:18:56Z

Since the convo on #3058 stalled, I just want to confirm:

Are we OK with proceeding with a format which is not (easily) checkable on-chain?

One thing for sure is that the infix format (current status quo) is something that needed to be removed/deprecated. Other options can be explored/discussed, but the current conditions format could not remain due to the ambiguity of logical operations and the ongoing technical debt of also supporting infix. Moving from infix notation to prefix notation wasn't too bad of a lift.

#3058 was mainly about infix vs prefix, and expanded into a discussion about the language used for conditions. Totally understandable since they are in the same realm, but ultimately the condition language itself is a much broader discussion than intended for that issue, and perhaps we can continue that higher level conversation in a more focused issue since there are still many unknowns to resolve/consider:

Can something like a BytecodeCondition type simply just be another type of condition like RPC/Time/Contract - could that then be a decision of adopters to use instead of us forcing it?
How does it affect our goal for multichain capabilities?
What if you need compound conditions across different non-compatible chains? You're back to needing a common language to allow for compounded conditions across these anyway...
Do we need conditions to be verifiable by an on-chain referee?
Is it compatible with multicall work (@manumonti , @cygnusv )?
How does not using a JSON language affect the UX? How easily can apps generate the bytecode(s) they need for the condition(s)?

Filed #3144 to continue that discussion.

manumonti

I really like this prefix syntax for compound conditions. In my opinion, the code is pretty much clean in this way and error-proof. Great work. Just some thoughts in my review.

nucypher/policy/conditions/lingo.py

tests/unit/conditions/test_compound_conditions.py

nucypher/policy/conditions/lingo.py

manumonti · 2023-06-14T19:13:09Z

Is it compatible with multicall work (@manumonti , @cygnusv )?

I do not see any inconvenience for multicall using this format. Actually, the simplicity of this code is better for implementing multicall.

…ogical conditions.

…ate_condition_lingo`.

…propagated but gets reraised as InvalidConditionLingo.

…erators. Co-authored-by: David Núñez <david@nucypher.com>

manumonti

LGTM! ✌️

examples/tdec/simple_tdec.py

KPrasch · 2023-06-15T10:19:34Z

nucypher/characters/lawful.py

-
-        ciphertext = self.encrypt_for_dkg(plaintext=plaintext,
-                                          conditions=conditions)
+    def encrypt_for_dkg_and_produce_decryption_request(


Needs a better name IMO, but outside the scope of this PR.

nucypher/network/server.py

KPrasch · 2023-06-15T10:30:43Z

nucypher/policy/conditions/rust_shims.py

-import json
-from typing import List
-
-from nucypher_core import ReencryptionRequest, Conditions
-
-from nucypher.policy.conditions.lingo import ConditionLingo
-
-
-def _serialize_rust_lingos(lingos: List[Conditions]) -> Conditions:
-    lingo_lists = list()
-    for lingo in lingos:
-        if lingo:
-            lingo = json.loads((str(lingo)))
-        lingo_lists.append(lingo)
-    rust_lingos = Conditions(json.dumps(lingo_lists))
-    return rust_lingos
-
-
-def _deserialize_rust_lingos(reenc_request: ReencryptionRequest):
-    """Shim for nucypher-core lingos"""
-    json_lingos = json.loads(str(reenc_request.conditions))
-    lingo = [ConditionLingo.from_list(lingo) if lingo else None for lingo in json_lingos]
-    return lingo


Such a relief to finally realize this removal!

KPrasch

Excellent PR 👏🏻 I'm in favor of merging, although since this is mission-critical code that has gained complexity (rightfully so) and the conditions format is predictable we might be able to develop a fuzzing strategy to ensure logical correctness in all possible cases.

nucypher/network/retrieval.py

nucypher/network/server.py

piotr-roslaniec · 2023-06-15T12:18:44Z

nucypher/policy/conditions/lingo.py

-            _operator = data["operator"]
-        except KeyError:
-            raise InvalidLogicalOperator(f"Invalid operator data: {data}")
+class CompoundAccessControlCondition(AccessControlCondition):


Not specific to this PR:

Should we put a ceiling on the number of elements in conditions? Something in the order of ~100. We could short-circuit it too: evaluate the first 100, and if there exists another then return False.

Seems like a possible DoS vector. WDYT?

That's a very sensible restriction, in fact I'd make the ceiling much lower, perhaps 5-10 elements since each call adds o(n) runtime at minimum, and maps directly onto node operational costs. Even beyond that, we may consider some form of rate limiting. All of which needs to be addressed in another PR IMO.

Nice thought!

We can potentially limit the length of the list of conditions, and raise an InvalidCondition exception if too long during object creation/schema validation.

I'd rather do that than allow the object to be created then fail during execution because it is too long - could be too confusing to the user. That being said, technically you can work around it by having repeated nested compound conditions each with operands under the limit.

So maybe some kind of overall count needs to be calculated during creation...?

Some good thoughts here. Let's tackle it in another PR.

There's definitely both client and server validation involved there, I think this is often the case with the condition module as a whole.

I agree that's material for another contribution. I'll be interested in scoping this, also for the purpose of implementation in nucypher-ts.

👍 - filed #3148.

Just to throw is out there, another way to enforce a limitation is by capping the number of bytes.

piotr-roslaniec

Just a couple of questions.

LGTM 🎉

derekpierre added a commit to derekpierre/nucypher that referenced this pull request Jun 11, 2023

Add newsfragments for nucypher#3140.

4633e35

derekpierre changed the title ~~[WIP] Prefix notation for logical compound conditions~~ Prefix notation for logical compound conditions Jun 12, 2023

derekpierre requested review from jMyles, KPrasch, cygnusv, theref, manumonti and piotr-roslaniec June 12, 2023 13:20

derekpierre self-assigned this Jun 12, 2023

derekpierre marked this pull request as ready for review June 12, 2023 13:20

derekpierre mentioned this pull request Jun 12, 2023

Prefix notation for logical condition operators nucypher/taco-web#224

Closed

derekpierre force-pushed the prefix-conditions branch from 8d16f25 to ed9121f Compare June 12, 2023 18:15

This was referenced Jun 12, 2023

To bytecode or not to bytecode...that is the question #3144

Closed

Versioned Condition Lingo #3145

Merged

[WIP] Prefix notation for conditions #3031

Closed

manumonti reviewed Jun 14, 2023

View reviewed changes

nucypher/policy/conditions/lingo.py Show resolved Hide resolved

nucypher/policy/conditions/lingo.py Show resolved Hide resolved

tests/unit/conditions/test_compound_conditions.py Outdated Show resolved Hide resolved

nucypher/policy/conditions/lingo.py Show resolved Hide resolved

derekpierre and others added 9 commits June 14, 2023 19:38

Initial work that replaces infix notiation with prefix notation for l…

610f743

…ogical conditions.

Add tests/fixes for compound conditions.

4214a9b

Localize condition lingo handling/processing to one function - `evalu…

45a420a

…ate_condition_lingo`.

Add newsfragments for nucypher#3140.

ff57ca1

Ensure that the underlying marshmallow schema ValidationError is not …

c4b217e

…propagated but gets reraised as InvalidConditionLingo.

Minor text cleanup.

f006dab

Remove use of eval/eval strings in favour of python or/and boolean op…

3cdee2c

…erators. Co-authored-by: David Núñez <david@nucypher.com>

Fix comments based on feedback from nucypher#3145.

13db8d2

Remove redundant Condition type object in lingo module.

42cf842

derekpierre force-pushed the prefix-conditions branch from 3e482b1 to 42cf842 Compare June 14, 2023 23:38

manumonti approved these changes Jun 15, 2023

View reviewed changes

KPrasch reviewed Jun 15, 2023

View reviewed changes

KPrasch approved these changes Jun 15, 2023

View reviewed changes

piotr-roslaniec reviewed Jun 15, 2023

View reviewed changes

nucypher/network/retrieval.py Show resolved Hide resolved

piotr-roslaniec reviewed Jun 15, 2023

View reviewed changes

nucypher/network/server.py Outdated Show resolved Hide resolved

piotr-roslaniec reviewed Jun 15, 2023

View reviewed changes

nucypher/network/server.py Show resolved Hide resolved

piotr-roslaniec reviewed Jun 15, 2023

View reviewed changes

piotr-roslaniec approved these changes Jun 15, 2023

View reviewed changes

Apply RFCs from nucypher#3140.

68b4ea7

derekpierre mentioned this pull request Jun 15, 2023

Limit CompoundAccessControlCondition to prevent overloading Ursulas #3148

Open

derekpierre merged commit 074dbc3 into nucypher:development Jun 15, 2023

derekpierre mentioned this pull request Jun 15, 2023

Support prefix notation for compound conditions nucypher/taco-web#226

Merged

8 tasks

KPrasch pushed a commit that referenced this pull request Nov 30, 2023

Add newsfragments for #3140.

c20f3c0

KPrasch pushed a commit that referenced this pull request Nov 30, 2023

Apply RFCs from #3140.

a2e0492

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefix notation for logical compound conditions #3140

Prefix notation for logical compound conditions #3140

derekpierre commented Jun 9, 2023 •

edited

Loading

jMyles commented Jun 12, 2023

derekpierre commented Jun 12, 2023 •

edited

Loading

manumonti left a comment

manumonti commented Jun 14, 2023

manumonti left a comment

KPrasch Jun 15, 2023

KPrasch Jun 15, 2023

KPrasch left a comment

piotr-roslaniec Jun 15, 2023

KPrasch Jun 15, 2023

derekpierre Jun 15, 2023

piotr-roslaniec Jun 15, 2023

derekpierre Jun 15, 2023

KPrasch Jun 15, 2023

piotr-roslaniec left a comment

Prefix notation for logical compound conditions #3140

Prefix notation for logical compound conditions #3140

Conversation

derekpierre commented Jun 9, 2023 • edited Loading

jMyles commented Jun 12, 2023

derekpierre commented Jun 12, 2023 • edited Loading

manumonti left a comment

Choose a reason for hiding this comment

manumonti commented Jun 14, 2023

manumonti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec left a comment

Choose a reason for hiding this comment

derekpierre commented Jun 9, 2023 •

edited

Loading

derekpierre commented Jun 12, 2023 •

edited

Loading