Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow various methods to accept a union of Expr, int, and/or str #152

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Dec 30, 2021

Currently, any method that expects a pyteal.Bytes or pyteal.Int object will throw an exception when a str or int is given. This can make some method calls more verbose than they really need to be. For example, App.globalGet("Key") is code that one would reasonable expect to work, but since "key" is a str and not pyteal.Bytes, it throws an exception .

This PR implements a function, get_teal_type that will return a pyteal.Int or pyteal.Bytes object given a int or str, respectively. As demonstrated in the test, App.globalGet("Key") now behaves the same as App.globalGet(Bytes("Key")).

Caveats

  • allow_redefinition = True was added to mypy.ini to prevent mypy from complaining about incorrect types being passed down. Since get_teal_type will always return a valid type or throw an exception this isn't a legitimate concern. It's possible that the implementation of these calls could be properly casted to appease mypy, but I think allowing redefinition is a more elegant solution. The reason mypy doesn't allow redefinition by default is to improve readability, but having a bunch of casts would likely hurt readability more than help it.

  • I also put this function in it's own file because I wasn't sure where else it would fit. If there's a preexisting file that this could go into, we should probably do so.

@StylishTriangles
Copy link
Contributor

I don't like the idea of mixing abstraction layers like this. Sure it's convenient but there is a subtle problem when trying to convert between representations of data. Consider:
s = "A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0"
What is the meaning of s value in TEAL? Is it an address? Maybe it's hex encoded? Or maybe it's just a string?
Bytes, Addr allow to distinguish those representations of data.

With this PR if you're not fluent in TEAL/PyTEAL it will be hard to tell if
App.localGet(Int(0), "A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0")
and
App.localGet(Int(0), Txn.accounts[1]) (assuming A0A0... is in foreignAccounts[1])
result in the same data being accessed

However using Addr("A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0") pretty much guarantees it will behave as expected.

There are a couple of things you can do right now to make the experience better:

  1. Name the constants App.localGet(SOME_ADDRESS, SOME_KEY)
  2. Write your own abstraction layer on top of PyTEAL

I'd be on board if the Union allowed bytes instead of str, then there would be no confusion towards the representation of data in tEAL, but then it'd be less convenient in some cases :)

@joe-p
Copy link
Contributor Author

joe-p commented Jan 11, 2022

With this PR if you're not fluent in TEAL/PyTEAL it will be hard to tell if App.localGet(Int(0), "A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0") and App.localGet(Int(0), Txn.accounts[1]) (assuming A0A0... is in foreignAccounts[1]) result in the same data being accessed

I could say the same thing with explicit Bytes: One might assume that App.localGet(Int(0), Bytes("A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0A0")) and App.localGet(Int(0), Txn.accounts[1]) access the same data (I did when first starting with Teal/pyTeal), but they don't.

To me, your comment addresses Bytes vs Addr rather than str implicitly being converted to Bytes. To clarify any confusion in your example, you'd just have to know that any str will be interpreted as Bytes.

If this PR was trying to turn str into Bytes or Addr I would understand your concern, but that is not the case. The only new information a developer would need to know is that str is always implicitly converted to Bytes.

@StylishTriangles
Copy link
Contributor

After thinking about this, I think you have a point.

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.

2 participants