-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: runtime error #14
feat: runtime error #14
Conversation
…ives_conversion refactor: pop primitives conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely looks better! We still have to do a little bit of thinking regarding the versioning. Then it is time to finalise, write docs and finish :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oke I think we have the implementation now. Polishing it up, testing and last step; writing clear documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding work! This is so much nicer than where we started, so kudos!
I have made various suggestions to try and simplify it a little more for a cleaner final product. I suspect some may not be possible, but if so it will reduce the amount of code required. Type names used by the end user should be as concise as possible so its less to type, quicker to achieve more.
PS - please ensure that any types used are consistent across runtime error and macro (e.g. module error first, then api error). I think the PR description has them in a different order, but that may not exist in the code.
Cargo.toml
Outdated
@@ -56,6 +54,8 @@ sp-runtime-interface = { version = "28.0.0", features = ["std"] } | |||
# Local | |||
drink = { path = "crates/drink/drink" } | |||
ink_sandbox = { path = "crates/ink-sandbox" } | |||
pallet-api = { path = "../pop-node/pallets/api" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reminder that these will need to be updated to the actual repos before this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man we are close! Amazing work! This will become incredibly important for the devex that pop claims to provide!
I'm not sure if anyone else made a comment that this crate should be generic in its explanation but because this is called pop-drink I guess we can opinionate a little bit and simplify. We want to provide an easy to understand test suite for developers building smart contracts on Pop Network using the pop api. Lets make it like that.
The term API is used for both pop api and apis in general. Lets make a clear distinction. The pop api is never clearly mentioned btw as well as the pop api error and the status code. The latter two might be a bit too low level but a link / reference is always good if they want to learn more.
Try to take the stand of a developer never having seen the psp22 example, pop api and pop drink crate. Viewing it by looking in the branch, not the PR (really taking the perspective of someone skimming through our repos), and see if there are some missing links and easy explanations that can bind all of it together nicely (not talking about extra work on pop api crate :) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last iteration for the docs.
Lets take a step back and try to explain in easy language what the crate provides to developers and what it can be used for.
One idea; put the things in order in the file first in terms of what the developer will want to use first. This the macro as first. Explain what the macro does. Then one layer deeper, the error type, explain that. Really try to explain the thing itself and leaving out the details when you go one layer deeper.
crates/pop-drink/src/error.rs
Outdated
/// Error::Module(Balances(InsufficientBalance)) | ||
/// ``` | ||
/// | ||
/// ### API errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this term API error make any sense to you? I find it very confusing to be honest. There is the Error
type which makes a separation between module runtime errors and the other errors that can come from the runtime. Everywhere where the latter is mentioned we call it ApiError. If this refers to the pop api error it is incorrect as the module error is basically also from the pop api error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by removing all the introduction of ApiError
ApiError -> RuntimeCallError
I changed it to RuntimeCallError
and don't mention anything about errors returned by the API. I reordered the RuntimeCallError
and ModuleError
so developers will read the RuntimeCallError
description first before visiting ModuleError
. From my perspective, ModuleError
is simply a subset of reasons why the runtime call fails, originating from the modules.
Hence, both errors types RuntimeCallError
and ModuleError
come from the runtime. Simply speaking, RuntimeCallError
is a parent of ModuleError
, but ModuleError
helps to test the runtime module error in a better way.
crates/pop-drink/src/error.rs
Outdated
ModuleError: Decode + Encode + Debug, | ||
ApiError: Decode + Encode + Debug + From<u32> + Into<u32>, | ||
{ | ||
/// Converts an `Error` into a numerical value of `ApiError`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is again confusing, why is it only a numerical value of ApiError
? It is a numerical value of Error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Resolved!
crates/pop-drink/src/error.rs
Outdated
ModuleError: Decode + Encode + Debug, | ||
ApiError: Decode + Encode + Debug + From<u32> + Into<u32>, | ||
{ | ||
/// Converts a numerical value of `ApiError` into an `Error`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again confusing for the same reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
crates/pop-drink/src/error.rs
Outdated
} | ||
} | ||
|
||
/// Asserts that a `Result` with an error type convertible to `u32` matches the expected `Error` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These documentation section also need to be revisited.
For example the first line:
Asserts an error type to Error
. This can be used for custom error types used by a contract which uses the status code returned by the pop runtime. The error type must be convertible to a u32
.
I think it is much easier to use the status code as a way of explaining everything here then by calling it an Api error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Resolved and removed all the introduction of ApiError
.
crates/pop-drink/src/error.rs
Outdated
/// | ||
/// # Examples | ||
/// | ||
/// The below example interacts with a [PSP22](https://github.com/w3f/PSPs/blob/master/PSPs/psp-22.md) contract that uses [Pop API](https://github.com/r0gue-io/pop-node/tree/main/pop-api). The contract method returns the API error [`PSP22Error`](https://github.com/r0gue-io/pop-node/blob/main/pop-api/src/v0/fungibles/errors.rs#L73C1-L73C22) which is provided by Pop API library. Learn more in the [PSP22 example contract](https://github.com/r0gue-io/pop-node/blob/main/pop-api/examples/fungibles/lib.rs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are referring to the psp22 error as the API error but it is very unclear why and how. The reason it is (in the current terminology) an API error is because it is an error that is converted from a status code (an error from the runtime) and has a variant that holds this status code. Information is extracted from this status code to see what runtime error happened. drink provides a simplified runtime error type which you can use to discover which error occurred.
Try to take a step back and explain the pop drink features to someone that is new to polkadot / smart contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved! Simplify the example by removing the PSP22Error and just return StatusCode instead: 30405b4#:~:text=///%20//%20Required%20imports%20to,Api(Arithmetic(Overflow)))%3B
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!
How to assert non-custom error thrown by Pop API?
This example interacts with a PSP22 example contract that uses Pop API. The contract method returns
PSP22Error
which is provided by Pop API library.Learn more in the PSP22 example contract.
transfer
method of thePsp22
contract:A custom error is returned if the error doesn't conform to the
PSP22Error
standard.The custom error is represented by a
StatusCode
, which encapsulates au32
value indicating the success or failure of a runtime call via the Pop API.Pop DRink! provides an error type and a macro to simplify testing both runtime module errors and API errors.
drink::v0::Error
: Runtime error type for testing API V0.assert_err
: Asserts that aResult
with an error type convertible tou32
matches the expectedError
from pop-drink.Test the contract call if a custom error is the runtime module error:
Test the contract call if a custom error is the API error:
Implementation
assert_runtime_error
RuntimeError
Introducing a new error type
RuntimeError
With two conversion methods
u32
: Used to compare theRuntimeError
with the another error type implementingInto<u32>
(See PSP22Error in pop-node)u32
: Used to convert a compared value intoRuntimeError
and display back to the developers.