-
Notifications
You must be signed in to change notification settings - Fork 87
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
Support StateProof keys. #778
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.
Generally looks good to me, though I can't give great feedback on some indexer-specific testing changes, like if it's ok to modify createTxn
.
Also note that the go-algorand submodule was updated to use the branch from algorand/go-algorand#2990, so probably that needs to be merged and the submodule changed back to master (or stable?) before this PR should be merged
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.
Thanks for the PR. The code looks good overall, but the compact cert transaction type needs to be added in idb/txn_type_enum.go
as discussed.
compact cert will be part of a different release. I was told to include keyreg changes only. |
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.
LGTM. We will need to wait for state proof changes to arrive in go-algorand before merging.
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.
LGTM. But please don't merge it in yet, we're still finalizing a release.
@@ -571,47 +582,34 @@ func TestFetchAccountsRewindRoundTooLarge(t *testing.T) { | |||
func createTxn(t *testing.T, target string) []byte { |
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.
Could you add a line like defer assert.Fail("this method should only be used for generating test inputs.")
so that we don't accidentally use this function in real tests?
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.
Adding this thing makes the CI fail.
do you have a different suggestion for that?
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 function being called? It looks like a helper for adding a new test.
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.
The idea is to keep the code that generates the test_resource.
I couldn't find more functions like this. Do you think it is worth keeping? (not as a Test function?)
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.
Yes, it is worth keeping.
addNewTest
above is set to false, so this function is never called. It should never be called in a finished test which is why I suggested putting assert.Fail
in here.
api/handlers_e2e_test.go
Outdated
@@ -300,6 +302,97 @@ func TestPagingRootTxnDeduplication(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestTransactionHandler(t *testing.T) { |
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.
Could you change this test name to reflect that you're testing the keyreg transaction?
var stateProofPK merklekeystore.Verifier | ||
stateProofPK[0] = 1 | ||
|
||
txn := transactions.SignedTxnWithAD{ |
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 updated MakeSimpleKeyregOnlineTxn
but it doesn't seem to be used anywhere. You could repurpose it for this test, or remove it.
Codecov Report
@@ Coverage Diff @@
## develop #778 +/- ##
===========================================
+ Coverage 58.85% 59.02% +0.17%
===========================================
Files 37 37
Lines 4421 4425 +4
===========================================
+ Hits 2602 2612 +10
+ Misses 1510 1499 -11
- Partials 309 314 +5
Continue to review full report at Codecov.
|
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.
only one small comment
idb/postgres/postgres.go
Outdated
@@ -1200,6 +1202,10 @@ func (db *IndexerDb) yieldAccountsThread(req *getAccountsRequest) { | |||
if hasVote { | |||
part.VoteParticipationKey = ad.VoteID[:] | |||
} | |||
if hasStateProofkey { | |||
sprfkey := ad.StateProofID[:] |
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.
to be safe, I would explicitly allocate new memory with new()
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.
Thanks, replaced this with our byteSlicePtr
helper.
Summary
Add new field, StateProofKey, to key reg txn fields and account participation. Accounts data written to postgres is also updated accordingly to include the additional field.
Test Plan
Add new unit and e2e tests for api handler to validate that StateProofKey is returned correctly in response.