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: Reorganize the crypto opcodes a bit to simplify incentive work #5787

Merged
merged 3 commits into from Oct 18, 2023

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Oct 17, 2023

Improves type checking a bit, improves ease of unit tests for new opcodes, moves crypto into their own files, adds some coverage.

@jannotti jannotti self-assigned this Oct 17, 2023
@jannotti jannotti marked this pull request as ready for review October 17, 2023 20:20
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #5787 (0fcda56) into master (6c482ab) will increase coverage by 0.03%.
The diff coverage is 87.43%.

@@            Coverage Diff             @@
##           master    #5787      +/-   ##
==========================================
+ Coverage   55.55%   55.58%   +0.03%     
==========================================
  Files         474      475       +1     
  Lines       66850    66850              
==========================================
+ Hits        37138    37161      +23     
+ Misses      27188    27180       -8     
+ Partials     2524     2509      -15     
Files Coverage Δ
crypto/onetimesig.go 76.15% <ø> (ø)
data/transactions/logic/doc.go 70.96% <ø> (ø)
data/transactions/logic/eval.go 92.85% <ø> (+0.54%) ⬆️
data/transactions/logic/opcodes.go 79.84% <ø> (ø)
data/transactions/logic/crypto.go 87.43% <87.43%> (ø)

... and 16 files with indirect coverage changes

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

@jannotti jannotti changed the title AVM: Reorganize the crypto opcodes a bit to simply incentive work AVM: Reorganize the crypto opcodes a bit to simplify incentive work Oct 18, 2023
Copy link
Member

@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 mostly good, I just have some minor questions/comments.

I also verified the new crypto.go contains exactly the code removed from eval.go

data/transactions/logic/doc.go Outdated Show resolved Hide resolved
data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/evalStateful_test.go Outdated Show resolved Hide resolved
data/transactions/logic/evalStateful_test.go Show resolved Hide resolved
data/transactions/logic/eval_test.go Outdated Show resolved Hide resolved
data/transactions/logic/assembler_test.go Show resolved Hide resolved
data/transactions/logic/README.md Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM, one question about a newly added notrack below

data/transactions/logic/eval.go Show resolved Hide resolved
data/transactions/logic/crypto_test.go Show resolved Hide resolved
@jannotti jannotti merged commit 26facb5 into algorand:master Oct 18, 2023
18 checks passed
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.

None yet

3 participants