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

AVM: Modify StackType to provide additional information #5130

Merged
merged 63 commits into from
May 10, 2023

Conversation

barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Feb 10, 2023

This pr modifies the existing StackType to allow more precise specification of types, especially those that are common, including a name and value or length bounds.

e.g.
address is a []byte that is exactly 32 bytes long
boolean is a uint64 that is in the range 0-1 (inclusive)

This change also has the benefit of allowing some additional assembly time checks. For example the TEAL snippet

bytes 0xdeadbeef
itxn_field Sender

should fail at assembly time because the byte string we're passing couldn't possibly be an address type.

Major Changes:

  1. The existing StackType is now called avmType and is a member of the new StackType struct.
  2. The proto function that provides typing for the ops has been updated to account for the new StackTypes
  3. A number of the OpSpecs have had their proto argument updated to denote they work on the more specific type.
  4. A number of Field specs have been updated to specify that they return the more specific type.
  5. The opdoc program has been updated to make use of the new types and fully document them in the langspec and README

@barnjamin barnjamin changed the title initial addition of new StackType AVM: initial addition of new StackType Feb 10, 2023
@barnjamin barnjamin changed the title AVM: initial addition of new StackType AVM: Modify StackType to provide additional information Feb 10, 2023
…s/Returns, fix some tests to use avm type for comparison
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #5130 (d9ca502) into master (f4f5ec6) will increase coverage by 1.57%.
The diff coverage is 74.40%.

@@            Coverage Diff             @@
##           master    #5130      +/-   ##
==========================================
+ Coverage   53.78%   55.35%   +1.57%     
==========================================
  Files         450      454       +4     
  Lines       56201    63898    +7697     
==========================================
+ Hits        30226    35373    +5147     
- Misses      23626    26127    +2501     
- Partials     2349     2398      +49     
Impacted Files Coverage Δ
agreement/agreementtest/simulate.go 86.66% <ø> (+1.14%) ⬆️
cmd/goal/application.go 18.19% <ø> (+0.40%) ⬆️
config/localTemplate.go 60.60% <ø> (-2.66%) ⬇️
daemon/algod/api/server/v2/handlers.go 0.86% <0.00%> (+0.02%) ⬆️
daemon/algod/api/server/v2/utils.go 11.94% <0.00%> (+1.02%) ⬆️
daemon/algod/server.go 4.30% <0.00%> (+0.38%) ⬆️
data/bookkeeping/genesis.go 26.38% <0.00%> (-0.28%) ⬇️
data/transactions/application.go 61.53% <0.00%> (-4.42%) ⬇️
data/transactions/logic/fields.go 75.50% <ø> (+0.37%) ⬆️
data/transactions/logic/tracer.go 50.00% <0.00%> (-7.15%) ⬇️
... and 44 more

... and 397 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@barnjamin barnjamin marked this pull request as ready for review February 10, 2023 18:08
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I have to go to a meeting, so this isn't much feedback yet. Main idea that I think would be helpful is if we didn't have to name some of these types. It forced you to shoehorn some things into "hash" or "addr" that really aren't. I'd rather they appear in docs as [32]byte (which would still be a nice improvement over []byte.

I wonder about some others, like key and bigint, would they be better as [..64]byte or something like that? I haven't yet seen, but I assume you have a new section in docs that explains these types?

data/transactions/logic/README.md Outdated Show resolved Hide resolved
data/transactions/logic/README.md Outdated Show resolved Hide resolved
data/transactions/logic/README.md Outdated Show resolved Hide resolved
data/transactions/logic/README.md Show resolved Hide resolved
data/transactions/logic/README.md Show resolved Hide resolved
data/transactions/logic/README.md Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/assembler.go Outdated Show resolved Hide resolved
data/transactions/logic/assembler.go Outdated Show resolved Hide resolved
data/transactions/logic/assembler.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/langspec.json Outdated Show resolved Hide resolved
data/transactions/logic/langspec.json Outdated Show resolved Hide resolved
data/transactions/logic/langspec.json Outdated Show resolved Hide resolved
data/transactions/logic/langspec.json Outdated Show resolved Hide resolved
@barnjamin barnjamin requested a review from jannotti April 4, 2023 14:27
@barnjamin barnjamin requested a review from jannotti May 8, 2023 19:04
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Seems like a good improvement, I just have minor comments

data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/eval.go Outdated Show resolved Hide resolved
data/transactions/logic/doc.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Outdated Show resolved Hide resolved
jannotti
jannotti previously approved these changes May 9, 2023
| any | | any |
| address | len(x) == 32 | []byte |
| []byte | len(x) <= 4096 | []byte |
| [32]byte | len(x) == 32 | []byte |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the [32]byte line and just hard code a line like:
| [N]byte | len(x) == N | []byte |

I'd probably remove the method time and use [4]byte directly if we did that.

Just looking for thoughts, no need to make changes here. I want to get this in.

data/transactions/logic/eval.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good, just one nit, but still looks good to merge

}

func (st StackType) constant() (uint64, bool) {
if st.Bound[0] == st.Bound[1] {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this function should return false if the stack type is any, based on prior discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants