search

ignite / cli

Ignite is a CLI tool and hub designed for constructing Proof of Stake Blockchains rooted in Cosmos-SDK
https://ignite.com
Other
1.26k stars 549 forks source link

Decide what to do with Cosmos-SDK errors #2802

Open aljo242 opened 2 years ago

aljo242 commented 2 years ago

The defined error types (such as ErrInsufficientFunds) still exist under the cosmos/cosmos-sdk path.

@aljo242 What's the reason behind this separation between error functions and error definitions ? I find easier to have everything in the same package.

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 https://github.com/ignite/cli/issues/2794#issuecomment-1233692296

aljo242 commented 2 years ago

In summary, cosmos-sdk has moved their errors package, only keeping the functionality - no defined error types are included. It seems like their recommendation is for chains to define all the errors themselves in appropriate codespaces.

We need to move to the cosmossdk.io/errors package (because the other is deprecated), but the current solution I have in #2794 is certainly not ideal.

We could:

  1. keep things as I have them in #2794
  2. Have chains define all of the errors in a package that is scaffolded in them
  3. Create a package in cli that registers the errors in the "cosmos-sdk" subspace and have chains import this
  4. Create a new repo ignite/sdk-errors that registers the errors in the "cosmos-sdk" subspace and have chains import this

I think the best solution is 4, but let me know what y'all think.

aljo242 commented 2 years ago

Another option is that we do nothing for the time being, but it does look like the plan is to remove the old types/errors package at some point.

tbruyelle commented 2 years ago
  1. keep things as I have them in refactor: use cosmossdk.io/errors package #2794

We'll have to do something at some point, so let's do it now since we have the time.

  1. Have chains define all of the errors in a package that is scaffolded in them

This is my preferred solution. The current types/errors/errors.go is copied in a CLI template and scaffolded in any new chain.

3) Create a package in cli that registers the errors in the "cosmos-sdk" subspace and have chains import this

Let's not add more CLI dependencies in scaffolded chain :P

4) Create a new repo ignite/sdk-errors that registers the errors in the "cosmos-sdk" subspace and have chains import this

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 ErrTxInMempoolCache, what will they do with them ? It's definitely not an error that should be defined in a chain.

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.

aljo242 commented 2 years ago

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 ErrTxInMempoolCache, what will they do with them ? It's definitely not an error that should be defined in a chain.

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.

aljo242 commented 2 years ago

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 cli so we will need to register our own errors for those as well.

lumtis commented 2 years ago

Option 3 sounds good to me, but how should and where we add these definitions to chains? We also use the sdk error types ourselves in cli so we will need to register our own errors for those as well.

This is a good solution. Not sure neither why those have been removed in the new package since something like invalidAddress is quite general.

Referring to chains should be registering their own errors we should rather go with solution 2 but we should avoid being unnecessarily too verbose in the scaffolding.

From the PR you created https://github.com/ignite/modules/pull/38 using this package in the templates looks like a good solution to me as ignite/module will intend to provide many utilities for SDK development. Developers are free to define their own error afterward. Scaffolding commands are not final, scaffolded code is aimed to be modified by developers afterward

aljo242 commented 2 years ago

@lubtd do you mean having scaffolded chains import ignite/modules/errors package?

lumtis commented 2 years ago

Yes, the other way would be to generate a package error general for the chain like we do with testutil but I'm not a big fan generating folder in user repo

ilgooz commented 2 years ago

I would say a combo of 2 and 3, where 3 is for common errors and 2 is for chain specific errors.

tac0turtle commented 2 years ago

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