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

Create ckb-error and use it as the global system error type #1386

Merged
merged 13 commits into from
Sep 9, 2019

Conversation

keroro520
Copy link
Contributor

@keroro520 keroro520 commented Aug 11, 2019

  • Create ckb-error and use it as the global system error type

  • This PR is an update of [HOLD] feat: create ckb-error #1344 based on [HOLD] feat: create ckb-error #1344 (comment)

  • There are several error kinds, and the corresponding error types:

    • OutPoint: util/types/src/core/error.rs
    • Transaction: verification/src/error.rs
    • Script: script/src/error.rs
    • Header: verification/src/error.rs
    • Block: verification/src/error.rs
    • Internal: error/src/internal.rs
    • Dao: util/dao/utils/src/error.rs
    • Spec: spec/src/error.rs

This PR does not cover the error types in ckb-miner, ckb-rpc and ckb-sync. Perhaps do it later.


NOTE:

  • I will remove the NOTE: the original name is * lines later. These lines are used to help review.

@keroro520 keroro520 requested review from quake, doitian, u2 and a team August 11, 2019 17:46
@nervos-bot
Copy link

nervos-bot bot commented Aug 11, 2019

@quake is assigned as the chief reviewer

@keroro520 keroro520 changed the title Ckb error kind [HOLD] Create ckb-error and use it as the global system error type Aug 13, 2019
nervos-bot[bot]
nervos-bot bot previously requested changes Aug 13, 2019
Copy link

@nervos-bot nervos-bot bot left a comment

Choose a reason for hiding this comment

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

Hold as requested by @keroro520.

@doitian doitian added the s:waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Aug 20, 2019
@keroro520 keroro520 changed the title [HOLD] Create ckb-error and use it as the global system error type Create ckb-error and use it as the global system error type Sep 3, 2019
@keroro520 keroro520 added s:waiting-on-reviewers Status: Waiting for Review and removed s:waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 3, 2019
sync/src/relayer/error.rs Outdated Show resolved Hide resolved
sync/src/synchronizer/headers_process.rs Outdated Show resolved Hide resolved
verification/src/error.rs Show resolved Hide resolved
verification/src/error.rs Outdated Show resolved Hide resolved
@keroro520
Copy link
Contributor Author

@u2 Thanks for your careful review. I have updated according to the above suggestions, in the new commitment 029e902

  • Rename some fields
  • Distinguish Pow and Epoch errors
  • Explicitly list the malformed errors

@keroro520 keroro520 requested a review from u2 September 5, 2019 06:46
verification/src/error.rs Outdated Show resolved Hide resolved
error/src/internal.rs Outdated Show resolved Hide resolved
script/src/error.rs Outdated Show resolved Hide resolved
@zhangsoledad
Copy link
Member

I oppose all TooMuch TooMany-series rename

@keroro520
Copy link
Contributor Author

@zhangsoledad @u2 @quake

I would suggest changing the error names and descriptions back to the original. And then open another issue to discuss it. What are your opinions?

@u2
Copy link
Contributor

u2 commented Sep 9, 2019

bors r=quake,u2,zhangsoledad

@nervos-bot nervos-bot bot added the s:ready-to-merge Status: Waiting to be merged. label Sep 9, 2019
bors bot added a commit that referenced this pull request Sep 9, 2019
1386: Create ckb-error and use it as the global system error type r=quake,u2,zhangsoledad a=keroro520

* Create ckb-error and use it as the global system error type
* This PR is an update of #1344 based on #1344 (comment)

* There are several error kinds, and the corresponding error types:

  * OutPoint: `util/types/src/core/error.rs`
  * Transaction: `verification/src/error.rs`
  * Script: `script/src/error.rs`
  * Header: `verification/src/error.rs`
  * Block: `verification/src/error.rs`
  * Internal: `error/src/internal.rs`
  * Dao: `util/dao/utils/src/error.rs`
  * Spec: `spec/src/error.rs`

This PR does not cover the error types in ckb-miner, ckb-rpc and ckb-sync. Perhaps do it later.


----

NOTE:

- I will remove the `NOTE: the original name is *` lines later. These lines are used to help review.

Co-authored-by: keroro520 <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 9, 2019

Canceled

@u2
Copy link
Contributor

u2 commented Sep 9, 2019

bors r=quake,u2,zhangsoledad

bors bot added a commit that referenced this pull request Sep 9, 2019
1386: Create ckb-error and use it as the global system error type r=quake,u2,zhangsoledad a=keroro520

* Create ckb-error and use it as the global system error type
* This PR is an update of #1344 based on #1344 (comment)

* There are several error kinds, and the corresponding error types:

  * OutPoint: `util/types/src/core/error.rs`
  * Transaction: `verification/src/error.rs`
  * Script: `script/src/error.rs`
  * Header: `verification/src/error.rs`
  * Block: `verification/src/error.rs`
  * Internal: `error/src/internal.rs`
  * Dao: `util/dao/utils/src/error.rs`
  * Spec: `spec/src/error.rs`

This PR does not cover the error types in ckb-miner, ckb-rpc and ckb-sync. Perhaps do it later.


----

NOTE:

- I will remove the `NOTE: the original name is *` lines later. These lines are used to help review.

Co-authored-by: keroro520 <[email protected]>
@zhangsoledad
Copy link
Member

bors r=quake,u2,zhangsoledad

bors bot added a commit that referenced this pull request Sep 9, 2019
1386: Create ckb-error and use it as the global system error type r=quake,u2,zhangsoledad a=keroro520

* Create ckb-error and use it as the global system error type
* This PR is an update of #1344 based on #1344 (comment)

* There are several error kinds, and the corresponding error types:

  * OutPoint: `util/types/src/core/error.rs`
  * Transaction: `verification/src/error.rs`
  * Script: `script/src/error.rs`
  * Header: `verification/src/error.rs`
  * Block: `verification/src/error.rs`
  * Internal: `error/src/internal.rs`
  * Dao: `util/dao/utils/src/error.rs`
  * Spec: `spec/src/error.rs`

This PR does not cover the error types in ckb-miner, ckb-rpc and ckb-sync. Perhaps do it later.


----

NOTE:

- I will remove the `NOTE: the original name is *` lines later. These lines are used to help review.

Co-authored-by: keroro520 <[email protected]>
@zhangsoledad zhangsoledad removed the s:waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Sep 9, 2019
@bors
Copy link
Contributor

bors bot commented Sep 9, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 57aa831 into nervosnetwork:develop Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:ready-to-merge Status: Waiting to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants