-
Notifications
You must be signed in to change notification settings - Fork 549
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
Decide what to do with Cosmos-SDK errors #2802
Comments
In summary, cosmos-sdk has moved their We need to move to the We could:
I think the best solution is 4, but let me know what y'all think. |
Another option is that we do nothing for the time being, but it does look like the plan is to remove the old |
We'll have to do something at some point, so let's do it now since we have the time.
This is my preferred solution. The current
Let's not add more CLI dependencies in scaffolded chain :P
Better than 3) That being said, for the moment, the cosmos-sdk doesn't follow that recommandation since those errors are still used in many places in their codebase. In addition there's some protocol-oriented errors like So to conclude I'm not sure we have sufficient information about the future of those errors to be able to take the right decision. |
Yeah, this is confusing to me. There are many "top level" errors that make sense to be defined in an "sdk" codespace since they sort of apply globally to Cosmos SDK chains. |
Option 2 sounds good to me, but how should and where we add these definitions to chains? We also use the sdk error types ourselves in |
This is a good solution. Not sure neither why those have been removed in the new package since something like Referring to From the PR you created ignite/modules#38 using this package in the templates looks like a good solution to me as |
@lubtd do you mean having scaffolded chains import |
Yes, the other way would be to generate a package |
I would say a combo of 2 and 3, where 3 is for common errors and 2 is for chain specific errors. |
app chains should define their own code space and use those instead of the sdk. It is some extra boilerplate but helps in debug ability. Please don't provide them a package to import, this goes against what the sdk is trying to establish |
The main reasoning is that the package has been deprecated. The new package is located here and does not contain the registered error types.
I also agree that separating packages is not ideal. According to this note, chains should be registering their own errors. I'm not sure what to do with this, so maybe we should have a broader discussion.
Originally posted by @aljo242 in #2794 (comment)
The text was updated successfully, but these errors were encountered: