Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce size of Rust library #33

Merged
merged 5 commits into from
Sep 17, 2024
Merged

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Sep 11, 2024

Store ChainId as u64 and move network names to a constant. Currently it is generated as an inline array which is computed on every function call to get_network_names; this shows up in Foundry release binary as one of the biggest functions (using cargo-bloat):

 0.0%   0.2%  86.5KiB                     mesc mesc::network_names::get_network_names

Making ChainId a u64 simplifies a ton of code, I'm not sure why this was a String with conversions from/to integers in the first place rather than the opposite

This is a breaking change to the ChainId and get_network_names APIs to reduce allocations, but it doesn't have to be if you feel strongly about breaking changes.
Serde for ChainId is also unmodified.

@sslivkoff
Copy link
Member

moving network names to a constant is 👍 👍

for chain id's the current format is due to a few related factors

  1. the EVM allows 256 bit chain id's. even though they aren't commonly used, having this full bit range may be useful in the future for creating new networks dynamically. 256 bits also means that btc forks and solana fork chain id's can be supported in the future
  2. JSON's max int size is 2^53-1, so it can't even store u64, let alone u256
  3. it's desirable for mesc to have similar functionality across different languages beyond rust. using strings is an easy way to interface with languages that don't have a native u256 type

(there is also a question of whether it would be valuable to support cosmos chain_id's, which afaict are unbounded in size and require a string. but this is probably low priority)

given all of these factors, what do you think is the most reasonable path forward? could leave as is, could modify the rust client, or if there's a big possible improvement could modify entire mesc spec

@DaniPopes
Copy link
Member Author

I see, this doesn't modify serde or the spec (still hex number), just the in-memory representation. Currently there is no usage of >u64 but the implementation can be easily updated to support them when needed so I think this is fine

@sslivkoff sslivkoff merged commit 0b3150b into paradigmxyz:main Sep 17, 2024
5 checks passed
@DaniPopes DaniPopes deleted the smaller branch September 17, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants