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

Multivalue-opcodes #543

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

Conversation

PabloLION
Copy link
Contributor

@PabloLION PabloLION commented Sep 24, 2022

Before finishing the ergonomic part of Wide64 arithmetic in #541, the basic opcodes should be implement first.

Goal

Here I'm trying to add addw, mulw, divw(exsited), divmodw. And maybe some other stack ops like dig n, swap, cover n, uncover n.

TBD

  1. I didn't work on the stack ops like dig n, swap, cover n, uncover n manipulate stack hence are not very useful in pyteal, where we don't usually interact the stack.
  2. Structure & Naming: I created a new class WideExpr(MultiValue) in new file 'wideexpr.py', and named the methods as AddW, MulW, etc. instead of Addw, etc, since they are all multivariable functions (like addw is binary-multivalue), hence current system which categorizes functions based on their input var num (like 'binaryexpr', 'ternaryexpr', 'naryexpr') doesn't work well on them. The common property is that they all focus on WideInt, but this might breaks the current categorization to be not mutually exclusive and might leads to confusion.

Tasks

  • add addw
  • test addw for valid case
  • test addw with invalid case,
  • add Class WideExpr
  • add mulw, expw, divmodw with their tests
  • add function overload for addw (adding many 64bit uint)
  • test for addw overload for valid case and invalid case
  • add function overload for mulw (multiplying many 64bit uint)

It seems there's no arithmetic test in tests so I skip them too.
These parts I didn't finish are moved to PR#541 where I implement the ergo-features.

@PabloLION
Copy link
Contributor Author

With some vision from #541 , where I'm going to create a Wide64 type similar to MaybeValue in 'wide64.py', and for a w: Wide64, w.high() load the high word scratch, and w.low() the low.
With this in mind, I'm going to update the addw to return a more specific Wide64 instead of MultiValue. So these methods can be categorize to 'wideexpr.py'.
So I split them for now to a ' multiexpr.py' and rename it in the next branch.

@PabloLION PabloLION mentioned this pull request Sep 25, 2022
@PabloLION
Copy link
Contributor Author

PabloLION commented Sep 25, 2022

For DivW, I added a AVM version test and rename, but didn't move it. I think it's nicely categorized as ternary expr, since it doesn't "produce WideInt".

@PabloLION PabloLION marked this pull request as ready for review September 25, 2022 23:56
@PabloLION
Copy link
Contributor Author

PabloLION commented Sep 26, 2022

Would you mind reviewing this PR as well? @michaeldiamant
It precedes #541.

return super().__teal__(options)


WideExpr.__module__ = "pyteal"
Copy link
Contributor Author

@PabloLION PabloLION Sep 26, 2022

Choose a reason for hiding this comment

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

I don't understand this after reading PEP3130. I think it's rejected? I never used this personally, but I still added it as it's part of your convention. I hope I did it correctly.

@PabloLION PabloLION mentioned this pull request Sep 26, 2022
8 tasks
pyteal/ast/multi_test.py Outdated Show resolved Hide resolved
Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>
@michaeldiamant
Copy link
Contributor

@PabloLION Thanks for putting in some cycles here.

Due to other priorities, it's been some time since I last looked into the PR's topic. From my perspective, I see the ergonomics and API design as intertwined. Since pyteal makes API backwards compatibility guarantees, I'm hesitant to bring the PR in and consider ergonomics later.

I'm not looking to discourage you from continuing ahead. But, I want to be upfront that given the current priority set, we're unlikely to merge the PR in a near future.

@PabloLION
Copy link
Contributor Author

@michaeldiamant Thanks for the reply.
I appreciate the backwards compatibility too. Otherwise I would've changed the second overload of Bytes in "bytes.py" and put the encoding as the second variable 😅. And here's actually a breaking change on the naming of Divw. Maybe we should keep the old one and redirect DivW to it.
Also, I'm glad that you noticed that PR #544 cannot be finished before those "TBD"s are resolved. I'll try your recommendation on the pyteal-utils which I think solves my current problem. Thanks.

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.

None yet

3 participants