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

Patch: Potential fix for panic on call to block w/ group resource sharing. #1590

Merged
merged 2 commits into from
Dec 31, 2023

Conversation

gmalouf
Copy link
Contributor

@gmalouf gmalouf commented Dec 31, 2023

No description provided.

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. 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.

Copy link

codecov bot commented Dec 31, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (9ef21ad) 68.49% compared to head (50a40d7) 68.42%.

Files Patch % Lines
api/converter_utils.go 68.42% 5 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@gmalouf gmalouf merged commit 3e9446a into algorand:main Dec 31, 2023
4 of 6 checks passed
@gmalouf gmalouf deleted the resource-shared-accounts-indexing branch December 31, 2023 03:30
onetechnical pushed a commit that referenced this pull request Dec 31, 2023
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):
Copy link
Contributor

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 ?

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants