Refactoring Router taking CSP expression #636

ahangsu · 2023-01-18T17:41:53Z

An attempt made, so we can get around the erroring in csp.

Closes #638

ahangsu · 2023-01-19T15:50:34Z

pyteal/ast/router.py

+    def __post_init__(self):
+        if self.clear_state != CallConfig.NEVER:
+            raise TealInputError(
+                "Attempt to construct clear state program from MethodConfig, "
+                "use Router top level argument `clear_state` instead"
+            )
+


Question: this function will error if one sets clear_state to anything other than default. Are we good here, or any more steps needed to totally remove the keyword arg clear_state?

This seems sufficient to me

We might want to have a link to a URL that explains in more detail. But that can also be added in the followup pr/release.

I update the doc a little bit and referred to doc in 030eaf4, keep looking into what should be done for csp refactoring.

jasonpaulos · 2023-01-19T18:29:08Z

pyteal/ast/router.py

+    def __post_init__(self):
+        if self.clear_state != CallConfig.NEVER:
+            raise TealInputError(
+                "Attempt to construct clear state program from MethodConfig, "
+                "use Router top level argument `clear_state` instead"
+            )
+


This seems sufficient to me

pyteal/ast/router.py

jasonpaulos · 2023-01-19T18:34:40Z

Note that we will also need to update the docs, notably https://pyteal.readthedocs.io/en/stable/abi.html#registering-bare-app-calls

tzaffi · 2023-01-19T18:41:35Z

An attempt made, so we can get around the erroring in csp.

Closes #638

We can add usage examples to the PR description. Can be done after merging as well.

pyteal/ast/router.py

tzaffi

In addition to a URL, I left a point of discussion. But overall, looking good.

tzaffi · 2023-01-19T19:48:22Z

pyteal/compiler/compiler_test.py

-        @pt.ABIReturnSubroutine
-        def approve_if_odd(condition_encoding: pt.abi.Uint32) -> pt.Expr:


This deleted router method is a great example to think through.
In the short term, I support the current strict approach. But in the medium term, it's worth seeing if we can relax the restrictions a bit.
As we discussed, there could still be a use case when someone actually wants to fail during a clear state cleanup action. So the approve_if_odd can be a stand-in for that. How would we want to enable such a call? Some ideas:

Do we allow any arguments at all?

If so, what kind of checks for the arguments should we generate? In particular, what happens if the 0'th argument - for the method selector - is incorrect?

Should we allow an option that instead of issuing err issues int 1; return immediately instead?

These questions get kind of dicey, but I can imagine modifying @router.method (or maybe even introducing @router.clear_method) with some parameters that control this. EG:

for_clear: bool (unneeded in the case of @router.clear_method) indicates whether this is intended for the clear program, and then enables the following parameters:

no_erroring = True - in the case that the method selector (or any other parameter) is wrong, will issue an int 1; return instead of err. But this means that all the other @router.clear_method would need to have the same no_erroring value. So maybe the routing err behavior belongs as a router level option.

I think my take on the discussion is that, we might want to let router user to explicitly specify what kind of logic they want to use in clear state program (CSP), so my thoughts and opinions are as follows:

Do we allow any arguments at all?

There is no way we are disallowing args, one can always use Txn.application_args in their composed Expr. We will just no longer generate code to handle the IO process in CSP.

If so, what kind of checks for the arguments should we generate? In particular, what happens if the 0'th argument - for the method selector - is incorrect?

If we go down the path of letting users to specify whatever logic in CSP, then I don't see too good a way to generate checks for arguments.

For now, I haven't thought through how much flexibility and check can be done on our side, but rather, probably we want to hand the full control over CSP to router users?

pyteal/ast/router_test.py

pyteal/ast/router.py

jasonpaulos · 2023-01-20T19:47:19Z

I had some docs improvement ideas, so I opened #641

tzaffi

Minor grammatical suggestions and a controversial suggestion to default to generating int 1 for the clear program

pyteal/ast/router.py

tzaffi · 2023-01-20T20:08:03Z

pyteal/ast/router.py

        """

        self.name: str = name
        self.descr = descr

        self.approval_ast = ASTBuilder()
-        self.clear_state_ast = ASTBuilder()
+        self.clear_state: Expr = (


I believe this is backwards compatible in the sense that if we have a pyteal-program written today with no information about clear-state, it will generate an int 0 for its clear-program.

~~However, it is arguable that even in this default case, we still want a NEW behavior and produce int 1 instead for the clear program.~~

And assuming we agree with this point, then unfortunately, the parameter will need to become mandatory because of the lack of backwards compatibility. So keeping it as kwarg-only param, we would need to have something like:

assert clear_state, "... something similar to the other error messages above ..."

In either case, I encourage the creation of a tiny unit test (assuming it doesn't already exist) that will assert that the simplest compiling PyTeal program generates the approval and clear programs that we would like to generate.

Upon further reflection, I changed my mind.... int 0 for the simplest possible clear program still makes sense because there is no state that is changing inside of the program and that needs to be preserved.

Thought for a bit, we still keep the backwards compatible way, right?

One question: even one call a CSP with int 0, then it failed, it still get its state cleared, right?

jasonpaulos

It's nearly there for me, just waiting on the below comment and #636 (comment)

CHANGELOG.md

Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>

tzaffi

LGTM

ahangsu added 2 commits January 18, 2023 12:39

attempt to add csp default final branch

1c03b32

remove bunch of stuffs

60869ad

ahangsu marked this pull request as ready for review January 18, 2023 23:45

changelog

6cc75c2

ahangsu requested review from barnjamin, bbroder-algo, jasonpaulos and tzaffi January 19, 2023 01:28

ahangsu changed the title ~~Default CSP branch at the end of cond~~ Refactoring Router taking CSP expression Jan 19, 2023

update comments about deprecated kwarg

c0edc81

ahangsu commented Jan 19, 2023

View reviewed changes

update comments on new kwarg in router

b0039cb

ahangsu force-pushed the csp-default branch from 743a63e to b0039cb Compare January 19, 2023 16:00

jasonpaulos reviewed Jan 19, 2023

View reviewed changes

tzaffi reviewed Jan 19, 2023

View reviewed changes

pyteal/ast/router.py Show resolved Hide resolved

ahangsu added 2 commits January 19, 2023 14:31

Merge branch 'master' into csp-default

faa734f

changelog

9553c2e

tzaffi reviewed Jan 19, 2023

View reviewed changes

ahangsu added 4 commits January 19, 2023 15:13

method config clear state clear state config

7999c76

bare call failure on clear state

9585b07

wrap handler flexible error msg from arg

2575241

minor, testcase

b8c4ed1

ahangsu force-pushed the csp-default branch from e55f628 to b8c4ed1 Compare January 19, 2023 21:15

ahangsu added 2 commits January 20, 2023 13:39

document change

2a99594

document change reflected in error message

030eaf4

ahangsu requested review from jasonpaulos and tzaffi January 20, 2023 19:12

jasonpaulos reviewed Jan 20, 2023

View reviewed changes

pyteal/ast/router_test.py Outdated Show resolved Hide resolved

pyteal/ast/router.py Show resolved Hide resolved

pyteal/ast/router.py Outdated Show resolved Hide resolved

pyteal/ast/router.py Show resolved Hide resolved

Clear state docs changes

16b7f2b

jasonpaulos mentioned this pull request Jan 20, 2023

Clear state docs changes #641

Merged

barnjamin approved these changes Jan 20, 2023

View reviewed changes

missing error message

40b68f6

ahangsu force-pushed the csp-default branch from b3ad7a9 to 40b68f6 Compare January 20, 2023 20:00

Merge branch 'clear-state-docs' into csp-default

3908ee2

tzaffi reviewed Jan 20, 2023

View reviewed changes

router testcase

d887a84

ahangsu force-pushed the csp-default branch from 814ab49 to d887a84 Compare January 20, 2023 20:14

method decorator comment

8aa1c9d

ahangsu requested review from tzaffi and jasonpaulos January 20, 2023 20:55

jasonpaulos reviewed Jan 20, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

ahangsu added 2 commits January 20, 2023 17:33

error message checking

7003f85

CHANGELOG

d943b3f

jasonpaulos approved these changes Jan 20, 2023

View reviewed changes

tzaffi reviewed Jan 20, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

26e5a91

Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>

tzaffi approved these changes Jan 20, 2023

View reviewed changes

ahangsu merged commit 8752ad5 into master Jan 20, 2023

ahangsu deleted the csp-default branch January 20, 2023 23:00

ahangsu mentioned this pull request Feb 1, 2023

Adding asdict() to Router's BareCallActions #656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Router taking CSP expression #636

Refactoring Router taking CSP expression #636

ahangsu commented Jan 18, 2023 •

edited

ahangsu Jan 19, 2023

jasonpaulos Jan 19, 2023

tzaffi Jan 19, 2023

ahangsu Jan 20, 2023

jasonpaulos Jan 19, 2023

jasonpaulos commented Jan 19, 2023

tzaffi commented Jan 19, 2023

tzaffi left a comment

tzaffi Jan 19, 2023

ahangsu Jan 20, 2023

jasonpaulos commented Jan 20, 2023

tzaffi left a comment

tzaffi Jan 20, 2023 •

edited

tzaffi Jan 20, 2023

ahangsu Jan 20, 2023

jasonpaulos left a comment •

edited

tzaffi left a comment

		@pt.ABIReturnSubroutine
		def approve_if_odd(condition_encoding: pt.abi.Uint32) -> pt.Expr:

Refactoring Router taking CSP expression #636

Refactoring Router taking CSP expression #636

Conversation

ahangsu commented Jan 18, 2023 • edited

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

jasonpaulos commented Jan 19, 2023

tzaffi commented Jan 19, 2023

tzaffi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpaulos commented Jan 20, 2023

tzaffi left a comment

Choose a reason for hiding this comment

tzaffi Jan 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpaulos left a comment • edited

Choose a reason for hiding this comment

tzaffi left a comment

Choose a reason for hiding this comment

ahangsu commented Jan 18, 2023 •

edited

tzaffi Jan 20, 2023 •

edited

jasonpaulos left a comment •

edited