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.

Sign up for GitHub

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

Jump to bottom

panicError handling for hooks #363

Merged
merged 5 commits into from
Sep 5, 2022
Merged

panicError handling for hooks #363

merged 5 commits into from
Sep 5, 2022

Conversation

puneet2019
Copy link
Member

@puneet2019 puneet2019 commented Aug 24, 2022 •
edited
Loading

1. Overview

Handles panics in hooks functions

2. Implementation details

3. How to test/use

  • No tests, but code blocks panicing will not change state

4. Checklist

  • Does the Readme need to be updated?

5. Limitations (optional)

  • We still do not handle error

6. Future Work (optional)

  • Handle checks by changing hooks to return errors

@puneet2019 puneet2019 mentioned this pull request Aug 24, 2022
Closed
@github-actions
Copy link

Coverage after merging puneet/transferHooksError into master

29.17%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
simapp
   state.go100%100%0%..., 96, 97, 98, 99
   config.go100%100%56.10%..., 72, 73, 74, 75
   app.go100%100%80.98%..., 587, 588, 591, 592
   export.go100%100%15.27%..., 93, 97, 98, 99
   encoding.go100%100%100%
   test_suite.go100%100%0%..., 52, 53, 54, 56
   test_helpers.go100%100%0.98%..., 96, 97, 98, 99
   utils.go100%100%20.25%..., 96, 97, 98, 99
   genesis_account.go100%100%100%
   genesis.go100%100%100%
simapp/simd/cmd
   genaccounts.go100%100%72.44%..., 95, 97, 98, 99
   testnet.go100%100%79.44%..., 408, 409, 62, 63
   root.go100%100%72.69%..., 66, 67, 70, 71
x/epochs/client/cli
   query.go100%100%0%..., 95, 96, 97, 98
   tx.go100%100%0%..., 22, 23, 24, 25
x/epochs/keeper
   keeper.go100%100%92.86%31
   genesis.go100%100%90.91%14
   hooks.go100%100%100%
   abci.go100%100%100%
   epoch.go100%100%85.71%..., 26, 62, 88, 93
   grpc_query.go100%100%43.48%..., 47, 49, 50, 51
x/epochs/types
   hooks.go100%100%100%
   genesis.pb.go100%100%27.83%..., 96, 97, 98, 99
   genesis.go100%100%59.18%..., 57, 58, 61, 62
   query.pb.gw.go100%100%0%..., 96, 97, 98, 99
   keys.go100%100%0%20, 21, 22
   identifier.go100%100%0%..., 25, 7, 8, 9
   query.pb.go100%100%2.24%..., 96, 97, 98, 99
x/halving/simulation
   params.go100%100%100%
   genesis.go100%100%100%
x/halving/types
   query.pb.gw.go100%100%0%..., 95, 96, 97, 98
   halving.pb.go100%100%2.76%..., 96, 97, 98, 99
   genesis.go100%100%86.67%25, 26
   genesis.pb.go100%100%2.55%..., 68, 69, 98, 99
   query.pb.go100%100%1.91%..., 96, 97, 98, 99
   params.go100%100%85.29%23, 24, 25, 43, 44

@puneet2019 puneet2019 changed the title add error handling. [requires discussion] panicError handling for hooks Aug 24, 2022
@puneet2019
Copy link
Member Author

puneet2019 commented Aug 24, 2022 •
edited
Loading

We can also have hooks return error..
and

if err == nil { write() } //writes cacheCtx.

this way is more cleaner as it doesn't require me to panic in keeper code logic (keeper functions can be called by other functions in app)

  • Will require refactoring of hooks

Unknown - what happens to cacheCtx if returned without write()? I expect it to be dumped.

@puneet2019 puneet2019 force-pushed the puneet/transferHooksError branch from aa1ae8a to 3345b99 Compare September 5, 2022 11:03
@puneet2019 puneet2019 requested a review from Anmol1696 September 5, 2022 11:05
@puneet2019 puneet2019 marked this pull request as ready for review September 5, 2022 11:05
@github-actions
Copy link

github-actions bot commented Sep 5, 2022

Coverage after merging puneet/transferHooksError into master

50.34%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
simapp
   export.go100%100%15.27%..., 93, 97, 98, 99
   genesis.go100%100%100%
   test_helpers.go100%100%0.93%..., 96, 97, 98, 99
   config.go100%100%56.10%..., 72, 73, 74, 75
   test_suite.go100%100%0%..., 52, 53, 54, 56
   utils.go100%100%20.25%..., 96, 97, 98, 99
   genesis_account.go100%100%100%
   app.go100%100%77.41%..., 649, 650, 651, 652
   encoding.go100%100%100%
   state.go100%100%0%..., 96, 97, 98, 99
simapp/simd/cmd
   genaccounts.go100%100%72.44%..., 95, 97, 98, 99
   root.go100%100%72.69%..., 66, 67, 70, 71
x/epochs/client/cli
   query.go100%100%0%..., 95, 96, 97, 98
   tx.go100%100%0%..., 22, 23, 24, 25
x/epochs/keeper
   grpc_query.go100%100%43.48%..., 47, 49, 50, 51
   epoch.go100%100%85.71%..., 26, 62, 88, 93
   hooks.go100%100%100%
   abci.go100%100%100%
   keeper.go100%100%92.86%31
   genesis.go100%100%90.91%14
x/epochs/types
   identifier.go100%100%0%..., 25, 7, 8, 9
   keys.go100%100%0%20, 21, 22
   genesis.go100%100%59.18%..., 57, 58, 61, 62
   hooks.go100%100%100%
x/halving/simulation
   genesis.go100%100%100%
   params.go100%100%100%
x/halving/types
   genesis.go100%100%86.67%25, 26
   params.go100%100%85.29%23, 24, 25, 43, 44
x/interchainquery
   module.go100%100%62.50%..., 91, 94, 95, 96
   handler.go100%100%40%16, 17, 18
   genesis.go100%100%54.55%21, 22, 23, 24, 25
x/interchainquery/keeper
   keeper.go100%100%67.59%..., 64, 65, 87, 99
   queries.go100%100%98%66
   abci.go100%100%92.68%58, 59, 60
   grpc_query.go100%100%74.07%..., 33, 40, 43, 44
   msg_server.go100%100%68.33%..., 59, 60, 68, 69
x/interchainquery/types
   msgs.go100%100%56%..., 42, 48, 49, 50
   keys.go100%100%0%28, 29, 30
   codec.go100%100%100%
   genesis.go100%100%0%..., 4, 5, 8, 9

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

Coverage after merging puneet/transferHooksError into master

50.34%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
simapp
   test_helpers.go100%100%0.93%..., 96, 97, 98, 99
   encoding.go100%100%100%
   utils.go100%100%20.25%..., 96, 97, 98, 99
   config.go100%100%56.10%..., 72, 73, 74, 75
   state.go100%100%0%..., 96, 97, 98, 99
   export.go100%100%15.27%..., 93, 97, 98, 99
   test_suite.go100%100%0%..., 52, 53, 54, 56
   genesis.go100%100%100%
   app.go100%100%77.41%..., 649, 650, 651, 652
   genesis_account.go100%100%100%
simapp/simd/cmd
   genaccounts.go100%100%72.44%..., 95, 97, 98, 99
   root.go100%100%72.69%..., 66, 67, 70, 71
x/epochs/client/cli
   tx.go100%100%0%..., 22, 23, 24, 25
   query.go100%100%0%..., 95, 96, 97, 98
x/epochs/keeper
   abci.go100%100%100%
   keeper.go100%100%92.86%31
   genesis.go100%100%90.91%14
   grpc_query.go100%100%43.48%..., 47, 49, 50, 51
   hooks.go100%100%100%
   epoch.go100%100%85.71%..., 26, 62, 88, 93
x/epochs/types
   keys.go100%100%0%20, 21, 22
   identifier.go100%100%0%..., 25, 7, 8, 9
   hooks.go100%100%100%
   genesis.go100%100%59.18%..., 57, 58, 61, 62
x/halving/simulation
   genesis.go100%100%100%
   params.go100%100%100%
x/halving/types
   genesis.go100%100%86.67%25, 26
   params.go100%100%85.29%23, 24, 25, 43, 44
x/interchainquery
   handler.go100%100%40%16, 17, 18
   genesis.go100%100%54.55%21, 22, 23, 24, 25
   module.go100%100%62.50%..., 91, 94, 95, 96
x/interchainquery/keeper
   grpc_query.go100%100%74.07%..., 33, 40, 43, 44
   msg_server.go100%100%68.33%..., 59, 60, 68, 69
   queries.go100%100%98%66
   abci.go100%100%92.68%58, 59, 60
   keeper.go100%100%67.59%..., 64, 65, 87, 99
x/interchainquery/types
   msgs.go100%100%56%..., 42, 48, 49, 50
   keys.go100%100%0%28, 29, 30
   genesis.go100%100%0%..., 4, 5, 8, 9
   codec.go100%100%100%

@puneet2019 puneet2019 changed the title [requires discussion] panicError handling for hooks panicError handling for hooks Sep 5, 2022
Anmol1696
Anmol1696 approved these changes Sep 5, 2022
View reviewed changes
Copy link

@Anmol1696 Anmol1696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@puneet2019 puneet2019 merged commit 21cc577 into master Sep 5, 2022
puneet2019 added a commit to persistenceOne/pstake-native that referenced this pull request Sep 5, 2022
@puneet2019
hooks to return error - persistenceOne/persistence-sdk#363
6e33ee9
puneet2019 added a commit to persistenceOne/pstake-native that referenced this pull request Sep 5, 2022
@puneet2019
hooks to return error - persistenceOne/persistence-sdk#363 (#256)
c65746a
@puneet2019 puneet2019 deleted the puneet/transferHooksError branch October 20, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anmol1696 Anmol1696 Anmol1696 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@puneet2019 @Anmol1696