-
Notifications
You must be signed in to change notification settings - Fork 93
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
Patch: Potential fix for panic on call to block w/ group resource sharing. #1590
Patch: Potential fix for panic on call to block w/ group resource sharing. #1590
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.
Looks good. I'd kind of like to know why the old code is copying stxn.Txn.Account[k-1] instead of just returning it. But it seem it's being overzealous. If sdk.Address
is declared as an array (not slice), this has to be safe.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1590 +/- ##
==========================================
- Coverage 68.49% 68.42% -0.07%
==========================================
Files 37 37
Lines 7427 7434 +7
==========================================
Hits 5087 5087
- Misses 1911 1917 +6
- Partials 429 430 +1 ☔ View full report in Codecov by Sentry. |
Patch: Potential fix for panic on call to block w/ group resource sharing.
switch { | ||
case index == 0: | ||
return txn.Sender, nil | ||
case int(index-1) < len(txn.Accounts): |
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.
I wonder if this new logic should be conditional based on protocol version, @jannotti ?
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.
I'd say it's ok. This is a copy of the same code in go-algorand. My reasoning is that the old would error if the index was out of bounds. This code will too, unless the bug that caused a large index also created a SharedAccts array (which would not exist in old versions). So the behavior should always be the same as it was unless we have two coincidental bugs that happen to compensate for each other. In such a case, we return the account that has somehow made its way into SharedAccts
No description provided.