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

expose nonparticipation txn field in teal #2823

Merged
merged 6 commits into from
Sep 2, 2021

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented Sep 1, 2021

Summary

Expose the field Nonparticipation from KeyregTxnFields in teal. This is needed for checking keyreg txns' nonparticipation value in logicsigns.

Test Plan

Updated tests in logic

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.

For the most part looks good, except the new field should be enabled only for v5 and onward.

Also, we may want to make it such that LogicSigs running programs v4 and lower automatically reject transactions that contain true for the nonparticipation field, since they cannot inspect it. A similar thing was done when we introducing rekeying in TEAL v2. I'll see if I can find any relevant code.

edit: the relevant code is ComputeMinTealVersion in data/transactions/logic/eval.go. But since it's currently valid for programs to execute in the same group as nonparticipation keyreg txns, we'd probably need to introduce a new consensus param to make this change. I'm not sure if that would be worth it.

@@ -337,6 +337,7 @@ var txnFieldDocs = map[string]string{
"VoteFirst": "The first round that the participation key is valid.",
"VoteLast": "The last round that the participation key is valid.",
"VoteKeyDilution": "Dilution for the 2-level participation key",
"Nonparticipation": "Flag for participation key",
Copy link
Member

Choose a reason for hiding this comment

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

A better description might be "Key registration participation flag"

@@ -232,6 +235,7 @@ var txnFieldSpecs = []txnFieldSpec{
{LocalNumUint, StackUint64, 3},
{LocalNumByteSlice, StackUint64, 3},
{ExtraProgramPages, StackUint64, 4},
{Nonparticipation, StackUint64, 0},
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I think we'll need to make this first available in v5

@shiqizng
Copy link
Contributor Author

shiqizng commented Sep 1, 2021

For the most part looks good, except the new field should be enabled only for v5 and onward.

Also, we may want to make it such that LogicSigs running programs v4 and lower automatically reject transactions that contain true for the nonparticipation field, since they cannot inspect it. A similar thing was done when we introducing rekeying in TEAL v2. I'll see if I can find any relevant code.

edit: the relevant code is ComputeMinTealVersion in data/transactions/logic/eval.go. But since it's currently valid for programs to execute in the same group as nonparticipation keyreg txns, we'd probably need to introduce a new consensus param to make this change. I'm not sure if that would be worth it.

I don't think it would be worth a consensus upgrade either.

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.

Looks good. Two small recomendations.

Comment on lines 1890 to 1894
if txn.Nonparticipation {
sv.Uint = 1
} else {
sv.Uint = 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if txn.Nonparticipation {
sv.Uint = 1
} else {
sv.Uint = 0
}
sv.Uint = boolToUint(txn.Nonparticipation)

@@ -337,6 +337,7 @@ var txnFieldDocs = map[string]string{
"VoteFirst": "The first round that the participation key is valid.",
"VoteLast": "The last round that the participation key is valid.",
"VoteKeyDilution": "Dilution for the 2-level participation key",
"Nonparticipation": "Key registration participation flag",
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "Marks an account nonparticipating for rewards"?

That is similar to the language we use here: https://developer.algorand.org/docs/reference/transactions/

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #2823 (b00ffcd) into master (9f81a92) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
- Coverage   47.14%   47.11%   -0.04%     
==========================================
  Files         349      349              
  Lines       56430    56433       +3     
==========================================
- Hits        26606    26590      -16     
- Misses      26845    26861      +16     
- Partials     2979     2982       +3     
Impacted Files Coverage Δ
data/transactions/logic/doc.go 82.75% <ø> (ø)
data/transactions/logic/fields.go 83.58% <ø> (ø)
data/transactions/logic/fields_string.go 4.76% <0.00%> (-0.04%) ⬇️
data/transactions/logic/eval.go 90.33% <100.00%> (+0.01%) ⬆️
catchup/service.go 69.35% <0.00%> (-1.56%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-1.30%) ⬇️
ledger/blockqueue.go 83.90% <0.00%> (-1.15%) ⬇️
data/transactions/verify/txn.go 45.08% <0.00%> (-0.90%) ⬇️
ledger/acctupdates.go 62.13% <0.00%> (-0.42%) ⬇️
network/wsNetwork.go 60.90% <0.00%> (-0.19%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f81a92...b00ffcd. Read the comment docs.

@jannotti jannotti merged commit 632b835 into algorand:master Sep 2, 2021
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

5 participants