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

Refactor Uint8.set to make data size a first class concept #238

Conversation

michaeldiamant
Copy link
Contributor

Optionally proposes a refactoring of Uint8.set to isolate the storage data size as a first class concept.

Rationale:

  • Uint8 is my introduction to PyTeal ABI support. I found myself spending some time understanding Uint8.set implementation and incrementally refactored for my comprehension. It's quite possible that I'm just unfamiliar with the space.
  • In case folks feel it's a sufficient improvement, I thought I'd present the PR while the content is fresh in my mind. If we'd prefer to keep as is, I'll happily close the PR.

Notes:

  • I didn't extend the change to other Uint subclasses because it's unclear we want the change. If we want the change, I can extend the concept in a follow on PR.

@@ -3,3 +3,4 @@ mypy==0.931
pytest
pytest-timeout
py-algorand-sdk
types-dataclasses
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: Without installing types-dataclasses, Python 3.6 lacks @dataclass support.

Here's the error prior to installing the dependency: https://github.com/algorand/pyteal/runs/5471035129?check_suite_focus=true.

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaning towards not to add this, here are the reasons:

  • Seems Jason has a PR after the first version of ABI, this PR might induce merge conflict.
  • The introduction of SizedExpr is only for Uint, which is not a helpful abstraction can be extended to other implementation, and it somewhat obfuscates Expr and SizedExpr.
  • This leads to another pip package to be imported for backwards-compatability.

@michaeldiamant
Copy link
Contributor Author

@ahangsu Thanks for the quick feedback. Based on your thoughts, I'm closing the PR.

For completeness, I'm leaving inline thoughts below on the provided feedback.

Seems Jason has a PR after the first version of ABI, this PR might induce merge conflict.

Since I'm catching up, I hadn't seen #222 prior to opening the PR. Now, I see your point about overlap and merge conflicts.

The introduction of SizedExpr is only for Uint, which is not a helpful abstraction can be extended to other implementation, and it somewhat obfuscates Expr and SizedExpr.

Gotcha - A few points:

  • I'll need to learn more about the rest of the ABI type space to have a more informed view.
  • I agree the SizedExpr abstraction in its current form isn't worth retaining. Though there are some changes I'd eventually like to come back to: The refactoring eliminates some mutation and generally adds context for why size checks happen.

This leads to another pip package to be imported for backwards-compatability.

It's a good callout to consider whether or not the dependency is a worthwhile addition. Hopefully, we'll be moving to >= 3.8 shortly due to the Literal usage in #222.

@michaeldiamant michaeldiamant deleted the feature/abi-uint8_set branch March 10, 2022 16:00
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