Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented Jan 12, 2021

Makes a few helper error fn result types generic over the error type.

This helps to simplify using them with custom error types without resorting to Error -> String -> Error conversions, which loses the traceability / source errors.

polkadot companion: paritytech/polkadot#2254

@drahnr drahnr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B2-breaksapi labels Jan 12, 2021
@drahnr drahnr self-assigned this Jan 12, 2021
@drahnr drahnr force-pushed the bernhard-helper-fn-errors branch from 45b6ca2 to 9d86ee3 Compare January 12, 2021 14:10
Copy link

@ordian ordian left a comment

Choose a reason for hiding this comment

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

small nits, but looks good other than that

Co-authored-by: Andronik Ordian <write@reusable.software>
@drahnr drahnr added the B0-silent Changes should not be mentioned in any release notes label Jan 13, 2021
@drahnr drahnr requested review from a team and bkchr January 13, 2021 11:13
@tomusdrw
Copy link
Contributor

Can you outline the details of what we try to achieve here more clearly? On the first glance it doesn't seem very convenient to have the caller define the type - why couldn't the caller simply wrap a concrete ServiceError? I suppose the reason is that ServiceError conversion is loosing some details, but I as @cecton noticed it might be better to simply avoid loosing these details and then on the caller level wrap or unpack ServiceError.

Having the return type generic makes pretty confusing API and may sometimes require extra type annotations (i.e. you most likely can't do main(...)? when the error return type of main is generic).

@drahnr
Copy link
Contributor Author

drahnr commented Jan 13, 2021

Currently in polkadot there is a cli module with additional CLI parameters which re-uses parts of the substrate provided convenience fns. Now since some of those wrappers take closures, returning Result types with Error types partially from substrate and partially from polkadot, the current hack is to do the mentioned lossy conversion dance.

If the error types are generics, the user of the substrate lib has the power to define the wrapping type and avoid (i.e. )

https://github.com/paritytech/polkadot/pull/2254/files#diff-ec440f309e0f836fa78c89ed2ac8bacbca15d177c2826d8f01a1447b33d38d39L171-L180

From my pov substrate is a framework, so if there is user code that could potentially return other error types within that closure, we should provide a path / allow those to be propagated instead.

It's not about if we use sc_cli::Error or sc_service::Error, that was a step to reduce the trait bounds on the error generic E.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

@drahnr thanks for explanations! Indeed seems like the right way.

Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

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

OK I see the point now 🤗

andresilva and others added 6 commits January 13, 2021 14:19
* network-gossip: add metric for number of local messages

* grandpa: fix GossipEngine missing metrics registry parameter

* network-gossip: increase known messages cache size

* network-gossip: fix tests

* grandpa: remove unnecessary clone

Co-authored-by: Max Inden <mail@max-inden.de>

* network-gossip: count registered and expired messages separately

* network-gossip: add comment on known messages cache size

* network-gossip: extend comment with cache size in memory

Co-authored-by: Max Inden <mail@max-inden.de>
* Remove statistics system

* Remove ContextData struct

* Remove next_request_id

* Some TryFrom nit-picking

* Use constants for peer sets
…tem (#7879)

* Add `len` function that can return the length of a storage item efficiently

* Make use of the new len function in contracts

* Fix benchmarks

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* Remove unused imports

Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
* Fix clear prefix check to avoid erasing child trie roots.

* Renaming and extend existing test with check.

* last nitpicks.
@drahnr drahnr merged commit c2aa425 into master Jan 13, 2021
@drahnr drahnr deleted the bernhard-helper-fn-errors branch January 13, 2021 18:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants