feat: publish verify attestation #82
Conversation
| @@ -105,6 +105,7 @@ impl Client { | |||
| &self, | |||
| topic: Topic, | |||
| message: impl Into<Arc<str>>, | |||
| attestation: impl Into<Option<Arc<str>>>, | |||
There was a problem hiding this comment.
Option<impl T> is generally better for a public API, as Into<Option<A>> for Option<B> is usually not implemented.
There was a problem hiding this comment.
My thinking is that if you already have an Arc<str>, then you'd be able to omit Some(), but in practice that's probably never gonna happen.
I think the conversation to have here is why are we still sticking to Arc<str>, even though they likely don't have any significant performance benefits, and only hurt the ergonomics of the APIs.
There was a problem hiding this comment.
What would be the alternative to Arc<str>? I think it's good to have basically free cloning
There was a problem hiding this comment.
In practice we rarely clone these strings.
The downsides to using Arc<str> is double allocation on deserialization (it essentially first deserializes into String, and then converts that to Arc<str>, if I'm not mistaken) and the ergonomics of working with it (converting String to Arc<str> is another allocation).
An alternative is using regular String. The cloning cost is mostly mitigated by using a fast allocator like jemalloc or mimalloc.
It's hard to say definitively what would be more performant for the relay, but in some cases (a lot of highly multi-threaded cloning) using String is a lot faster, since allocators would use essentially thread-local memory arenas with minimal locking, while Arc<str> is using atomics under the hood for synchronization. And even though atomic usage is cheap, it's not free.
There was a problem hiding this comment.
double allocation on deserialization (it essentially first deserializes into String, and then converts that to Arc, if I'm not mistaken)
Oh wow I didn't know about that. Yeah that looks accurate
There was a problem hiding this comment.
I don't think it's the case, seems like it initially deserializes it into String, then converts it into Box<str> and then into Arc<str>
The reallocation can only happen if the underlying Vec shrinks (https://doc.rust-lang.org/std/vec/struct.Vec.html#method.shrink_to_fit) and I assume that's not the case, because the original String should have the exact size needed.
However, it may depend on the deserializer.
Description
This adds the necessary new fields for the verify attestation API.
How Has This Been Tested?
Relay integration.
Due Diligence