-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add stateproof keyreg field #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks good to me, just very minor comments.
@@ -192,7 +195,8 @@ export function makeKeyRegistrationTxnWithSuggestedParams( | |||
voteKeyDilution: any, | |||
suggestedParams: any, | |||
rekeyTo?: any, | |||
nonParticipation = false | |||
nonParticipation = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: does this need to have a default value? it looks like that value is always set in the other constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default value has runtime implications (i.e. JS will fill in false
if no value is provided when the function is called) while the other definitions are purely for static type analysis
@@ -206,6 +210,7 @@ export function makeKeyRegistrationTxnWithSuggestedParams( | |||
type: TransactionType.keyreg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you didn't make these changes, but do you know why type and reKeyTo are are labeled but the others are not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A label is not necessary if it's being set to a variable whose name is the same as the label. E.g. these are the same:
let a = 1;
let b = { a: a };
let c = { a }; // same as above
@@ -448,6 +489,7 @@ export class Transaction implements TransactionStorageStructure { | |||
!txn.nonParticipation && | |||
(txn.voteKey || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this check needed here. It's checking one of the fields is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to make sure if any of the online keyreg fields are provided, then all must be provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Though once stateproofs are merged into master, I would like to do a quick review again
572e925
to
acea9e4
Compare
Add stateproof key field to keyreg transaction, in line with the changes from algorand/go-algorand#2990