Skip to content

Enhancement: Add missing ArgEnum fields, tweaks to immediate note#4903

Merged
jannotti merged 4 commits intoalgorand:masterfrom
barnjamin:add-missing-langspec-fields
Dec 14, 2022
Merged

Enhancement: Add missing ArgEnum fields, tweaks to immediate note#4903
jannotti merged 4 commits intoalgorand:masterfrom
barnjamin:add-missing-langspec-fields

Conversation

@barnjamin
Copy link
Copy Markdown
Contributor

Updating the ImmediateNote fields to be a bit more consistent.

Adding ArgEnum handler for several ops that were missing their immediate argument options.

@barnjamin barnjamin added documentation Improvements or additions to documentation Enhancement labels Dec 14, 2022
@barnjamin barnjamin requested a review from jannotti December 14, 2022 13:54
Comment thread cmd/opdoc/opdoc.go
Comment thread data/transactions/logic/doc.go
Comment thread data/transactions/logic/doc.go
Comment thread data/transactions/logic/doc.go Outdated
Comment thread data/transactions/logic/doc.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 14, 2022

Codecov Report

Merging #4903 (d9b48d5) into master (bd64ce8) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4903      +/-   ##
==========================================
+ Coverage   54.08%   54.09%   +0.01%     
==========================================
  Files         427      427              
  Lines       53528    53528              
==========================================
+ Hits        28948    28958      +10     
+ Misses      22314    22305       -9     
+ Partials     2266     2265       -1     
Impacted Files Coverage Δ
data/transactions/logic/doc.go 61.53% <ø> (ø)
agreement/cryptoVerifier.go 67.60% <0.00%> (-2.12%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
network/wsPeer.go 67.06% <0.00%> (-1.91%) ⬇️
ledger/acctupdates.go 68.99% <0.00%> (-0.25%) ⬇️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
catchup/service.go 70.53% <0.00%> (+1.44%) ⬆️
ledger/blockqueue.go 88.50% <0.00%> (+2.87%) ⬆️
ledger/tracker.go 78.90% <0.00%> (+3.79%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

📣 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 December 14, 2022 14:23
Copy link
Copy Markdown
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 think it's general good. I have some notes.

Comment thread cmd/opdoc/opdoc.go
Comment thread data/transactions/logic/TEAL_opcodes.md Outdated
## block f

- Opcode: 0xd1 {uint8 block field}
- Opcode: 0xd1 {uint8 block field index}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with this change (and similar, above) I suppose, but don't love it. The word "index" helps to convey that the encoding here is a number that is an index into an arbitrary list of fields, defined elsewhere. Without it, a reasonable person could ask, "What do you mean, how do I encode a field?"

On the other hand, it's not as though having the word index there makes it obvious what's going on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

open to other suggestions, option?

Comment thread data/transactions/logic/doc.go Outdated
@barnjamin barnjamin requested a review from jannotti December 14, 2022 17:43
@jannotti jannotti merged commit c8121b0 into algorand:master Dec 14, 2022
@barnjamin barnjamin deleted the add-missing-langspec-fields branch December 14, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation Enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants