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

Add Compound error return (Spearbit #33) #1550

Merged
merged 3 commits into from
Dec 16, 2022
Merged

Conversation

pakim249CAL
Copy link
Contributor

Pull Request

Issue(s) fixed

This pull request fixes #1535

@pakim249CAL pakim249CAL linked an issue Dec 8, 2022 that may be closed by this pull request
@pakim249CAL pakim249CAL changed the title Add Compound error return Add Compound error return (Spearbit #33) Dec 8, 2022
Copy link
Collaborator

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

  • why don't you kept the previous errors, which were more explicit, and just added the error code ?

  • btw those need to be remove if we keep the new ones

@pakim249CAL
Copy link
Contributor Author

pakim249CAL commented Dec 9, 2022

  • why don't you kept the previous errors, which were more explicit, and just added the error code ?

    • btw those need to be remove if we keep the new ones

Actually, I think kicking back Compound's errors is more appropriate than using our own custom errors. It is already a given and obvious from a stack trace what function call has an error. I think reverting with a Compound Error provides a good understanding of the error to those that get it. @MathisGD

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

LGTM but I agree we must remove the old errors

@Rubilmax
Copy link
Collaborator

Rubilmax commented Dec 12, 2022

It is already a given and obvious from a stack trace what function call has an error

I'm not sure an integrator not compiling the contracts would have the same level of granularity on a fork (needs to be tested, but not a priority for now). Also, all integrators don't use forge and don't have such granular stack traces...

Which is the reason I stand for granular custom errors (instead of a generic CompoundError), still carrying the underlying compound execution code

@pakim249CAL
Copy link
Contributor Author

Sorry for the force push. I accidentally commited work intended for another branch, so I reverted.

@pakim249CAL pakim249CAL merged commit dba2c17 into upgrade-morpho-1 Dec 16, 2022
@pakim249CAL pakim249CAL deleted the fix/spearbit-33 branch December 16, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Compound revert reasons (Spearbit #33)
4 participants