ethereum / trin

An Ethereum portal client: a json-rpc server with nearly instant sync, and low CPU & storage usage
363 stars 111 forks source link

Move away from using Vec<u8> to represent raw content key/value #1404

Open morph-dev opened 2 weeks ago

morph-dev commented 2 weeks ago

Instead of using Vec<u8> to represent raw content keys/values, we should use something like alloy_primitives::Bytes because:

We should either wrap or name the type, to avoid potential confusion (e.g. see RawContentValue).

Some of the places:

etherhood commented 2 weeks ago

Can I take this one?

morph-dev commented 2 weeks ago

Can I take this one?

@etherhood, I would wait for #1403 to be merged first, as it does background for this work. You can probably start working on top of it, but you might have to merge and resolve conflicts if there are some comments/concerns there.

etherhood commented 1 week ago

I see its merged now, If it is fine with you, I will start on it

etherhood commented 6 days ago

I have added some commits in WIP PR #1431 I changed inner type of RawContentKey and fixed its uses across codebase.

Should I also change Vec type to Bytes in all places wherever Vec is being used, as I couldn't find RawContentKey being used in portalnet/src/find/query_info.rs, which you mentioned in original issue, but there are usage of Vec

morph-dev commented 6 days ago

Should I also change Vec type to Bytes in all places wherever Vec is being used, as I couldn't find RawContentKey being used in portalnet/src/find/query_info.rs, which you mentioned in original issue, but there are usage of Vec

I would say replacing one type at the time is good (unless they have overlapping uses).

Regarding portalnet/src/find/query_info.rs, The Vec<u8> in RecursiveFindContentResult should be replaces with RawContentValue, but that should be a separate PR.

etherhood commented 6 days ago

Okay thanks, that sounds about right. I think than current PR is ready for review. Can you please review it. Thanks