Type `ProviderError` is not public

[tdelabro]

In starknet-providers there is a ProviderError type being defined:

#[derive(Debug, Error)]
pub enum ProviderError {
    #[error(transparent)]
    ReqwestError(#[from] ReqwestError),
    #[error(transparent)]
    Serialization(SerdeJsonError),
    #[error("Deserialization error: {err}, Response: {text}")]
    Deserialization { err: SerdeJsonError, text: String },
    #[error(transparent)]
    StarknetError(StarknetError),
}

It is not reexporte in lib.rs

#![doc = include_str!("../README.md")]

mod provider;
pub use provider::Provider;

mod sequencer_gateway;
pub use sequencer_gateway::SequencerGatewayProvider;

pub mod jsonrpc;

So I cannot use it in my code:

	provider
		.call_contract(
			InvokeFunctionTransactionRequest {
				contract_address,
				entry_point_selector: selector!("get_user_information_from_github_handle"),
				calldata,
				signature: Default::default(),
				max_fee: Default::default(),
			},
			BlockId::Latest,
		)
		.await

The return type of this expression is Result<CallContractResult, ProviderError> but I cannot do anything with it because it is not exported.
I cannot wrap it in my own error type, neither can I write a From implementation, nor return it directly.

What should be done ?
Probably export it too, or make the submodules public instead of keeping them private

[xJonathanLEI]

Type exported in #185.

[tdelabro]

You are not supposed to be able to use private type in declaration of public function. Rust is designed to prevent you to do this.
But you still manage to achieve it through insufficient exports.

You should reexport everything public in your lib.rs with pub use provider::*; and manage your export granularity in the provider module itself. With pub(super) pub(crate)

The current implementation feels like an anti-pattern to me.
And it's probably the same in every submodule you have in starknet-rs

[tdelabro]

I think you need to think this ahead and do a bit of global refactoring in your code exports archi. Not just export this one because I asked you.

[tdelabro]

If you open a wider issue, listing all your sub-modules that need such a refacto, I can help by doing some contribution.

[xJonathanLEI]

The sequencer provider will be replaced with the JSON-RPC client eventually so I don't plan to make big changes on this part of the codebase.

I believe it's common practice in Rust to selectively export types to rename stuff for the public API.

But as always, you can fork the library and maintain your own version given the permissive licenses.

[tdelabro]

It's a common practice to rename during export.
But what you did is make part of your lib inaccessible to users.
This is not the kind of issue that should require me to fork your repo. It is just about making it usable, not changing its behavior.

I believe if you made this mistake once there, you probably did it somewhere else in the repo.
You can either:

• keep this pattern and manage problems as they come, when users are dedicated enough to open issues
• keep this pattern and do a huge review of your codebase to make sure there is no other situation where you made some type unavailable while exposing it in a function. And make sure to keep that in mind in any further iteration of this codebase
• abandon this pattern, manage exports as I advised you, and never be bothered by this again

Your call

[xJonathanLEI]

I would certainly love to have this issue solved once and for all.

But using the pattern as advised (uses pub use xxx::*) loses the ability to rename during export, and makes everything have long names inside modules. So it's a tradeoff to me.

[xJonathanLEI]

Relevant: https://internals.rust-lang.org/t/private-struct-returned-by-public-fn

[xJonathanLEI]

The lint hasn't been implemented yet:

rust-lang/rust#34537

[xJonathanLEI]

@tdelabro Appreciate the suggestion but I feel like that isn't the most optimal solution...

This library is modeled extensively after ethers-rs, which uses the same pattern of manually choosing what to export.

[tdelabro]

If the tradeoff is having something that works vs having long names it's a no-brainer for me.
But as long as you are confident not to let a similar situation happen again, you can do whatever you want :)

[xJonathanLEI]

I just checked again and it seems the private_in_public lint is already in stable Rust, but isn't able to capture this issue. I tried nightly Rust too but no luck.

The thing is Rust does not consider this to be a "private" type since it's declared with pub. It's simply not exported.