Enforce that all implementations of the VssHeaderProvider are Send+Sync.

[G8XSU]

• Enforce that all implementations of the VssHeaderProvider are Send+Sync.

[tnull]

Is this necessary or could we do a cast like as &(dyn VssHeaderProvider + Send + Sync) or similar at the site were we use it?

[G8XSU]

Hmm.. i thought it was nice to have to make it easier to use header providers by everyone, instead of having some headerproviders which are not send+sync.

as &(dyn VssHeaderProvider + Send + Sync)

I wasn't able to fix it just using that maybe i am missing something.
In VssStore in ldk-node we aren't using any headerprovider as of now, so VssClient is created with FixedHeaders as headerProvider.
And VssClient contains Arc<dyn VssHeaderProvider> as its member, somehow compiler isn't able to determine it as send + sync unless I mark the member explicitly as headers: Arc<dyn VssHeaderProvider + Send + Sync>

I am not sure if there is much difference in enforcing all impls as Send + Sync or marking it in vssClient as

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

Let me know if you are suggesting something else.

[tnull]

Hmm.. i thought it was nice to have to make it easier to use header providers by everyone, instead of having some headerproviders which are not send+sync.

Well, usually you'd avoid adding these markers as the compiler is mostly able to figure them out on its own, and there might be instances where someone wants to use VssHeaderProvider but doesn't require it to be Send+Sync.

I wasn't able to fix it just using that maybe i am missing something. In VssStore in ldk-node we aren't using any headerprovider as of now, so VssClient is created with FixedHeaders as headerProvider. And VssClient contains Arc<dyn VssHeaderProvider> as its member, somehow compiler isn't able to determine it as send + sync unless I mark the member explicitly as headers: Arc<dyn VssHeaderProvider + Send + Sync>

I am not sure if there is much difference in enforcing all impls as Send + Sync or marking it in vssClient as

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

Let me know if you are suggesting something else.

There is a difference, for one, as mentioned above, there might be instances where you don't need it to be Send+Sync and requiring it might lead to unnecessary complication and of course in terms of trait inheritance it's not super logical as generally VssHeaderProvider and threading-related traits are really more or less unrelated concepts.

I think usually it's preferred to just include the explicit + Send + Sync where needed, but there also might be exceptions to it, and also no big deal if you prefer otherwise. However you decide, happy to move forward with this PR, i.e., def. shouldn't be a blocker here.

[G8XSU]

I am ok with adding Send+Sync at place of use but as described above, couldn't make it work other than adding it at VssClient header_provider member.
We expect VssClient to be Send+Sync and VssHeaderProvider is a member in it, hence it should be Send+Sync.

I can add

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

if that makes sense.

[tnull]

Tentative ACK, but let me know if you decide to make further changes.

[G8XSU]

Will merge this to unblock : lightningdevkit/vss-server#32 and lightningdevkit/ldk-node#357.
If any further changes, we can do them as followup.

[tnull]

Will merge this to unblock : lightningdevkit/vss-server#32 and lightningdevkit/ldk-node#357.

Note that it hasn't actually been unblocked yet, presumably because we have yet to release 0.3.1?