-
Notifications
You must be signed in to change notification settings - Fork 4
[NONEVM-3352] [L4|CL86-21] Wrong or colliding error codes #507
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
[NONEVM-3352] [L4|CL86-21] Wrong or colliding error codes #507
Conversation
13308e0 to
b41ebc1
Compare
b41ebc1 to
d3f1661
Compare
|
|
||
| enum FeeQuoter_Error { | ||
| UnsupportedChainFamilySelector = 24800 // Facility ID * 100 | ||
| UnsupportedChainFamilySelector = FeeQuoter_FACILITY_ID * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer having the entire value here. That way you can just search for 24800 and go straight to the Error enum. Just personal preference though, i am not against this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but if you get error 24801, the query fails. You still have to search for '248'. It is true though that it would take you to the error enum instead of the FACILITY_ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request reorganizes and standardizes error code definitions across the CCIP codebase to resolve inconsistencies and overlaps. The changes implement a facility-based error code system where each component has a unique facility ID that is multiplied by 100 to create distinct error code ranges.
Key changes include:
- Replacing hardcoded error codes with facility-based enums using a consistent pattern (
FACILITY_ID * 100) - Moving error definitions from scattered constants to centralized enum-based structures
- Updating TypeScript wrappers to match the new Tolk contract error code patterns
- Adding facility name constants and consistent error code generation across all contracts
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/ccip/bindings/ownable2step/types.go | Updates error codes to use iota pattern starting at 20400 |
| pkg/ccip/bindings/ownable2step/exitcode_string.go | Regenerates string representations for new error code values |
| pkg/ccip/bindings/ocr/exitcode.go | Adds new OCR exit code definitions with facility-based ranges starting at 3100 |
| pkg/ccip/bindings/ocr/exitcode_string.go | Generates string representations for OCR exit codes |
| pkg/ccip/bindings/merkleroot/merkle_root.go | Adds exit code definitions for merkle root validation errors |
| pkg/ccip/bindings/merkleroot/exitcode_string.go | Generates string representations for merkle root exit codes |
| integration-tests/deployment/ccip/cs_test.go | Updates test to use new OCR error constant instead of hardcoded value |
| contracts/wrappers/libraries/ocr/MultiOCR3Base.ts | Adds facility-based error enum replacing individual constants |
| contracts/wrappers/libraries/ocr/ExitCodes.ts | Removes file containing hardcoded error constants |
| contracts/wrappers/libraries/merkle_proof/MerkleMultiProof.ts | Adds facility-based error definitions for merkle proof validation |
| contracts/wrappers/libraries/access/Ownable2Step.ts | Updates to use facility-based error code pattern |
| contracts/wrappers/libraries/Deployable.ts | Adds facility constants and error enum, reorganizes code structure |
| contracts/wrappers/ccip/Router.ts | Renames constants to follow consistent FACILITY_NAME/FACILITY_ID pattern |
| contracts/wrappers/ccip/Receiver.ts | Updates to use consistent facility naming and adds type versioning methods |
| contracts/wrappers/ccip/ReceiveExecutor.ts | New wrapper file with facility-based error definitions |
| contracts/wrappers/ccip/OnRamp.ts | Standardizes facility constants and adds formatting improvements |
| contracts/wrappers/ccip/OffRamp.ts | Consolidates facility constants and removes redundant ReceiveExecutor definitions |
| contracts/wrappers/ccip/MerkleRoot.ts | Standardizes facility constants and adds getter methods |
| contracts/wrappers/ccip/FeeQuoter.ts | Updates facility constants and getter method signatures |
| contracts/wrappers/ccip/CCIPSendExecutor.ts | Standardizes facility constant naming |
| contracts/tests/libraries/ocr/MultiOCR3Base.spec.ts | Updates to use namespaced error constants and adds facility ID test |
| contracts/tests/lib/merkle_proof/MerkleMultiProof.spec.ts | Adds facility ID validation test |
| contracts/tests/lib/deployable/Deployable.spec.ts | Adds comprehensive facility and error code validation tests |
| contracts/tests/lib/access/Ownable2Step.spec.ts | Adds facility ID validation test |
| contracts/tests/gas-report/ccip/messaging/OffRamp.spec.ts | Updates to use new FACILITY_ID constant name |
| contracts/tests/ccip/sendExecutor/SendExecutor.spec.ts | Restructures tests and adds facility validation |
| contracts/tests/ccip/router/Router.spec.ts | Consolidates and updates facility validation tests |
| contracts/tests/ccip/onramp/OnRamp.spec.ts | Updates facility validation tests |
| contracts/tests/ccip/merkleRoot/MerkleRoot.spec.ts | Adds comprehensive facility and error code tests |
| contracts/tests/ccip/feequoter/FeeQuoter.spec.ts | Adds facility validation tests and removes redundant getter tests |
| contracts/tests/ccip/feequoter/FeeQuoter.getters.spec.ts | Removes redundant facility/error code tests |
| contracts/tests/ccip/Receiver.spec.ts | Updates to use namespaced constants and adds validation tests |
| contracts/tests/ccip/OffRamp.spec.ts | Major refactoring to use namespaced imports and adds comprehensive facility tests |
| contracts/contracts/test/examples/ocr3_base.tolk | Updates to use new error enum instead of constants |
| contracts/contracts/test/examples/funding/withdrawable_wallet.tolk | Renames facility constant for consistency |
| contracts/contracts/lib/utils.tolk | Removes generic unused error constants |
| contracts/contracts/lib/ocr/types.tolk | Updates to use new error enum |
| contracts/contracts/lib/ocr/multi_ocr3_base.tolk | Updates all error references to use enum |
| contracts/contracts/lib/ocr/exit_codes.tolk | Removes file with old error constants |
| contracts/contracts/lib/ocr/errors.tolk | Adds new error enum with facility-based codes |
| contracts/contracts/lib/crypto/merkle_multi_proof.tolk | Adds facility constants and error enum |
| contracts/contracts/lib/access/ownable_2step.tolk | Replaces constants with facility-based error enum |
| contracts/contracts/examples/counter.tolk | Replaces removed constant with inline value |
| contracts/contracts/deployable/types.tolk | Adds facility constants and error enum |
| contracts/contracts/deployable/contract.tolk | Updates to use new error enum and adds getter methods |
| contracts/contracts/ccip/test/receiver/types.tolk | Adds facility constants |
| contracts/contracts/ccip/test/receiver/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/test/receiver/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/router/types.tolk | Adds facility constants |
| contracts/contracts/ccip/router/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/router/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/receive_executor/types.tolk | Adds facility constants |
| contracts/contracts/ccip/receive_executor/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/receive_executor/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/onramp/types.tolk | Adds facility constants |
| contracts/contracts/ccip/onramp/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/onramp/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/offramp/types.tolk | Adds facility constants |
| contracts/contracts/ccip/offramp/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/offramp/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/merkle_root/types.tolk | Adds facility constants |
| contracts/contracts/ccip/merkle_root/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/merkle_root/contract.tolk | Updates to use facility constants |
| contracts/contracts/ccip/fee_quoter/types.tolk | Adds facility constants |
| contracts/contracts/ccip/fee_quoter/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/fee_quoter/contract.tolk | Updates to use facility constants and removes duplicate constant |
| contracts/contracts/ccip/ccipsend_executor/types.tolk | Adds facility constants |
| contracts/contracts/ccip/ccipsend_executor/errors.tolk | Updates error enum to use facility constant |
| contracts/contracts/ccip/ccipsend_executor/contract.tolk | Updates to use facility constants and removes duplicate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vicentevieytes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work
372a253 to
3c3afd6
Compare
NONEVM-3352