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

goal: account info with deleted asset suppress error and better output #5504

Merged
merged 2 commits into from Jun 28, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Jun 27, 2023

Summary

Suppress the asset search error with a better output on deleted/unknown asset (404 resp on querying such asset from algod). An example output is shown as follows:

Created Assets:
	<none>
Held Assets:
	ID 1003, asset-a, balance 0 units
	ID 1004, <deleted/unknown asset>
	ID 1005, asset-c, balance 0 units
	ID 1006, asset-d, balance 0 units
Created Apps:
	<none>
Opted In Apps:
	<none>
Minimum Balance:	500000 microAlgos

Should resolve #5154.

Test Plan

An e2e test for goal account info with deleted asset.

@ahangsu ahangsu self-assigned this Jun 27, 2023
@ahangsu ahangsu marked this pull request as ready for review June 27, 2023 17:09
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #5504 (ae58e01) into master (df22f75) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5504      +/-   ##
==========================================
- Coverage   55.82%   55.80%   -0.03%     
==========================================
  Files         446      446              
  Lines       63224    63229       +5     
==========================================
- Hits        35292    35282      -10     
- Misses      25565    25576      +11     
- Partials     2367     2371       +4     
Impacted Files Coverage Δ
cmd/goal/account.go 13.67% <0.00%> (-0.09%) ⬇️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Do we have the same issue if you're opted into a deleted app?

@ahangsu
Copy link
Contributor Author

ahangsu commented Jun 27, 2023

I am not sure we have the same issue for opting into deleted app, for we do not ask for application info...

Do we want to support that?

@jasonpaulos
Copy link
Member

jasonpaulos commented Jun 27, 2023

I am not sure we have the same issue for opting into deleted app, for we do not ask for application info...

Do we want to support that?

We don't need to fetch the app params to display app local states (as @ahangsu points out), so deleted apps are a non-issue

@algorandskiy algorandskiy merged commit 22e0e09 into algorand:master Jun 28, 2023
17 of 18 checks passed
@ahangsu ahangsu deleted the goal-acct-asset-output-fmt branch June 28, 2023 14:16
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.

Error is shown during goal account info if an asset is deleted
5 participants