pyteal icon indicating copy to clipboard operation
pyteal copied to clipboard

Multivalue-opcodes

Open PabloLION opened this issue 3 years ago • 5 comments

Before finishing the ergonomic part of Wide64 arithmetic in https://github.com/algorand/pyteal/pull/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

  • [x] add addw
  • [x] test addw for valid case
  • [x] test addw with invalid case,
  • [x] add Class WideExpr
  • [x] 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 avatar Sep 24 '22 19:09 PabloLION

With some vision from https://github.com/algorand/pyteal/pull/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 avatar Sep 24 '22 19:09 PabloLION

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 avatar Sep 25 '22 23:09 PabloLION

Would you mind reviewing this PR as well? @michaeldiamant It precedes https://github.com/algorand/pyteal/pull/541.

PabloLION avatar Sep 26 '22 00:09 PabloLION

@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.

michaeldiamant avatar Sep 27 '22 20:09 michaeldiamant

@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 https://github.com/algorand/pyteal/pull/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.

PabloLION avatar Sep 27 '22 22:09 PabloLION