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

feat: CCIP-2612 update behavior of _trialExecute to return error response on failure #1183

Merged
merged 12 commits into from
Jul 17, 2024
Merged

feat: CCIP-2612 update behavior of _trialExecute to return error response on failure #1183

merged 12 commits into from
Jul 17, 2024

Conversation

defistar
Copy link
Contributor

CCIP-2612 update behavior of _trialExecute to return error response on failure

Motivation

Solution

@defistar defistar self-assigned this Jul 10, 2024
@defistar defistar added the do not merge Do not merge at this time label Jul 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 10, 2024 •
edited
Loading

LCOV of commit bd2c384 during Solidity Foundry #6615

Summary coverage rate:
  lines......: 98.7% (1851 of 1876 lines)
  functions..: 96.3% (341 of 354 functions)
  branches...: 90.3% (791 of 876 branches)

Files changed coverage rate: n/a

@defistar defistar requested a review from elatoskinas July 10, 2024 19:46
@defistar defistar marked this pull request as ready for review July 11, 2024 10:24
@defistar defistar requested review from makramkd, RayXpub and a team as code owners July 11, 2024 10:24
@defistar defistar requested a review from elatoskinas July 11, 2024 10:39
@defistar
Copy link
Contributor Author

@elatoskinas do we need to add tests for all error selectors? to assert for its value in data of the emitted event ?

@elatoskinas
Copy link
Collaborator

do we need to add tests for all error selectors? to assert for its value in data of the emitted event ?

Don't think so - the test you modified should be enough, to verify that errors that previously reverted now just emit the event.

We should verify though that we're able to retrieve the error reason + error data from the event emission

@defistar defistar added looking for review This needs more reviewers and removed do not merge Do not merge at this time labels Jul 11, 2024
@defistar defistar force-pushed the feature/CCIP-2612-MultiOffRampErrorHandling branch from 0bf1cf0 to 1f3ac71 Compare July 15, 2024 10:24
@defistar defistar merged commit f420af8 into ccip-develop Jul 17, 2024
107 checks passed
@defistar defistar deleted the feature/CCIP-2612-MultiOffRampErrorHandling branch July 17, 2024 16:04
0xnogo pushed a commit that referenced this pull request Jul 19, 2024
@defistar @app-token-issuer-infra-releng @0xnogo
feat: CCIP-2612 update behavior of _trialExecute to return error resp…
d459e2d 
…onse on failure (#1183)

CCIP-2612 update behavior of _trialExecute to return error response on
failure

## Motivation


## Solution

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RayXpub RayXpub RayXpub approved these changes

@elatoskinas elatoskinas elatoskinas approved these changes

@RensR RensR RensR approved these changes

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner

Assignees

@defistar defistar

Labels
looking for review This needs more reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@defistar @elatoskinas @RensR @RayXpub