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

HAL-13 panic is used for error handling #3263

Open
juniuszhou opened this issue Sep 18, 2022 · 3 comments
Open

HAL-13 panic is used for error handling #3263

juniuszhou opened this issue Sep 18, 2022 · 3 comments
Assignees
Labels
Peggy Team Peggy team task

Comments

@juniuszhou
Copy link
Contributor

juniuszhou commented Sep 18, 2022

Description:
Several instances of the panic function were identified in the codebase. They appear to be used to handle errors. This can cause potential issues, as invoking a panic can cause the program to halt execution and crash in some cases. This in turn can negatively impact the availability of the software for users.

Code Location:

1 ./x/ ethbridge / genesis.go :20: panic ( err ) 
2 ./x/ ethbridge / test / test_helpers.go :182: panic ("Failed to apply validator set updates ")
3 ./x/ ethbridge / test / test_helpers.go :233: panic ( err ) 
4 ./x/ ethbridge / test / test_helpers.go :289: panic ( err ) 
5 ./x/ ethbridge / test / test_helpers.go :293: panic (" Bech encoding doesn 't match reference ") 
6 ./x/ ethbridge / test / test_helpers.go :297: panic ( err ) 
7 ./x/ ethbridge / test / test_helpers.go :300: panic (" Bech decode and hex decode don 't match ") 
8 ./x/ ethbridge / keeper / keeper.go :113: panic ( err ) 
9 ./x/ ethbridge / types / msgs.go :81: panic ( err )
10 ./x/ ethbridge / types / msgs.go :183: panic ( err )
11 ./x/ ethbridge / types / msgs.go :344: panic ( err )
12 ./x/ ethbridge / types / msgs.go :353: panic ( err )
13 ./x/ ethbridge / types / msgs.go :392: panic ( err )
14 ./x/ ethbridge / types / msgs.go :402: panic ( err )
15 ./x/ ethbridge / types / msgs.go :441: panic ( err )
16 ./x/ ethbridge / types / msgs.go :451: panic ( err )
17 ./x/ ethbridge / types / msgs.go :491: panic ( err )
18 ./x/ ethbridge / types / msgs.go :501: panic ( err )
19 ./x/ ethbridge / types / msgs.go :558: panic ( err )
20 ./x/ ethbridge / types / msgs.go :568: panic ( err )
21 ./x/ ethbridge / types / msgs.go :597: panic ( err )
22 ./x/ ethbridge / types / msgs.go :607: panic ( err )
23 ./x/ ethbridge / types / msgs.go :636: panic ( err )
24 ./x/ ethbridge / types / msgs.go :646: panic ( err )
25 ./x/ ethbridge / types / msgs.go :680: panic ( err )
26 ./x/ ethbridge / types / claim.go :89: panic (" Error : Network Descriptor must be between 0 -9999 ")
27 ./x/ tokenregistry / module.go :72: panic ( err )
28 ./x/ tokenregistry / module.go :110: panic ( err )
29 ./x/ tokenregistry / keeper / migrations.go :22:panic ( err )
30 ./x/ tokenregistry / keeper / keeper.go :167: panic (" Invalid Denom for Get Double Peg ")
31 ./x/ tokenregistry / keeper / keeper.go :181: panic (" ë Invalid Denom for Set Double Peg ")
32 ./x/ tokenregistry / keeper / metadata.go :37: panic (" ë Unhandled Registry Error ")
33 ./x/ tokenregistry / keeper / metadata.go :70: panic ("Unexpected error from GetRegistryEntry ")
34 ./x/ tokenregistry / types / genesis.go :21: // panic ( err )
35 ./x/ tokenregistry / types / genesis.go :34: panic ( fmt . ë Sprintf (" Failed to get genesis state from app state : %s", err .Error () ) )
36 ./x/ tokenregistry / types / genesis.go :50: panic ( fmt . ë Sprintf (" Failed to get genesis state from app state : %s", err .Error () ) )
37 ./x/ tokenregistry / types / msgs.go :58: panic ( err )
38 ./x/ tokenregistry / types / msgs.go :111: panic ( err )
39 ./x/ tokenregistry / types / msgs.go :162: panic ( err )
40 ./x/ tokenregistry / types / msgs.go :202: panic ( err )
41 ./x/ tokenregistry / types / msgs.go :246: panic ( err )
42 ./x/ tokenregistry / types / msgs.go :295: panic ( err )
43 ./x/ tokenregistry / types / msgs.go :305: panic ( err )

Recommendation

Instead of using panics, custom errors should be defined and handled according to the Cosmos best practices.

@juniuszhou juniuszhou self-assigned this Sep 18, 2022
@juniuszhou
Copy link
Contributor Author

I don't understand why not panic, if there is a serous issue, we really need to stop the node, even the chain. then we should panic, in some context, return an error and continue doesn't make sense and chain will be messy situation.

@juniuszhou
Copy link
Contributor Author

I prefer we still use panic and won't fix.

@pandaring2you
Copy link
Contributor

@smartyalgo asked for clarification from halborn on the appropriate time to panic since panics are used in cosmos sdk as well. michael to follow up. remaining item: review areas to determine if we actually need a panic.

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

No branches or pull requests

3 participants