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

Use proper SystemError::NoSuchContract on ContractInfo if missing #687

Closed
ethanfrey opened this issue Dec 1, 2021 · 10 comments · Fixed by #701
Closed

Use proper SystemError::NoSuchContract on ContractInfo if missing #687

ethanfrey opened this issue Dec 1, 2021 · 10 comments · Fixed by #701
Assignees

Comments

@ethanfrey
Copy link
Member

We return "normal" SDK errors rather than cosmwasm errors on many cases. This just gives strings to the contracts, which are not the most useful if we need to check if a contract is missing or not.

Let's make sure to return the proper wasmvm SystemErr type for these cases (at least missing contract).

Context (and workaround): https://github.com/confio/tgrade-contracts/pull/368/files#diff-25abef21c13511fbb128155747fbe8e908d0827b23e56ab0718c0c1b52acb44eR1206-R1215

@ethanfrey
Copy link
Member Author

For "proper" errors, see https://github.com/CosmWasm/wasmvm/blob/v0.16.0/types/systemerror.go#L8-L16

You need to do something like:

err := wasmvmtypes.SystemError{
  NoSuchContract: &wasmvmtypes.NoSuchContract{
    Addr: contractAddr,
  },
}

@ethanfrey
Copy link
Member Author

To see why it works when you return this type (and not sdk.Error), see:

RustQuery: https://github.com/CosmWasm/wasmvm/blob/v0.16.0/types/queries.go#L36-L52

ToQuerierResult: https://github.com/CosmWasm/wasmvm/blob/v0.16.0/types/queries.go#L60-L79

ToSystemError: https://github.com/CosmWasm/wasmvm/blob/12c336647f937183884fa52997ca9486d9a219e8/types/systemerror.go#L93

Actually, looking at this, we can simplify the above:

return nil, wasmvmtypes.NoSuchContract{ Addr: contractAddr }

@maurolacy maurolacy self-assigned this Dec 9, 2021
@ethanfrey
Copy link
Member Author

By not returning SystemErr, you fall into this case:

	return QuerierResult{
		Ok: &QueryResponse{
			Err: err.Error(),
		},
	}

Which says the contract was called and returned an error. We should use the SystemErrors when appropriate to communicate such info. Also any query to a non-existent contract should return this error, not 'not found' or empty bytes.

@ethanfrey
Copy link
Member Author

InvalidRequest and InvalidResponse are meant to be used for parsing errors.
Other ones are not needed AFAIK.

@alpe
Copy link
Contributor

alpe commented Dec 9, 2021

Thanks for the additional details!

@maurolacy
Copy link
Contributor

I would like to take a stab at fixing / standardizing this.

@maurolacy
Copy link
Contributor

maurolacy commented Mar 11, 2022

Note for a future improvement: This is still incomplete in that the (key) "not found" error is still untyped / generic.

@webmaster128
Copy link
Member

This is still incomplete in that the (key) "not found" error is still untyped / generic.

Which "not found" error are you referring to exactly? This one?

	// ErrNotFound error for an entry not found in the store
	ErrNotFound = sdkErrors.Register(DefaultCodespace, 8, "not found")

@maurolacy
Copy link
Contributor

This is still incomplete in that the (key) "not found" error is still untyped / generic.

Which "not found" error are you referring to exactly? This one?

	// ErrNotFound error for an entry not found in the store
	ErrNotFound = sdkErrors.Register(DefaultCodespace, 8, "not found")

I think so, yes.

@alpe
Copy link
Contributor

alpe commented Mar 14, 2023

This is true, I found some more cases that should be changed. I have opened #1258 to follow up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants