Skip to content

Comments

Add std::array support to BigInt::serialize function#5153

Open
KaganCanSit wants to merge 1 commit intorandombit:masterfrom
KaganCanSit:bigint-array-serialization
Open

Add std::array support to BigInt::serialize function#5153
KaganCanSit wants to merge 1 commit intorandombit:masterfrom
KaganCanSit:bigint-array-serialization

Conversation

@KaganCanSit
Copy link
Contributor

This PR resolves the TODO comment in BigInt::serialize() by adding support for std::array<uint8_t, N> alongside existing std::vector and secure_vector support.

TODO Message:

// TODO this supports std::vector and secure_vector
// it would be nice if this also could work with std::array as in
// bn.serialize_to<std::array<uint8_t, 32>>(32);

@coveralls
Copy link

coveralls commented Nov 9, 2025

Coverage Status

coverage: 90.337% (+0.001%) from 90.336%
when pulling 73d954c on KaganCanSit:bigint-array-serialization
into 90b85a8 on randombit:master.

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

Interesting way to detect an array! However, IIUC this would work for other tuple-types as well, no? Like std::pair<int, std::string>. That would produce very strange compiler errors for sure... But then again, the API does also allow passing a std::vector<uint32_t>, which is just as awful I guess. You could look into using the statically_spanable_range<> concept to detect the array instead.

Anyway: Could you please add a few tests that serialize a BigInt into an array? src/tests/test_bigint.cpp has a function test_bigint_serialization, please add another CHECK that uses std::array. Also check that the exception is thrown as expected if the array is too short or too long.

@KaganCanSit KaganCanSit force-pushed the bigint-array-serialization branch from cd14944 to 84439ab Compare November 12, 2025 18:51
@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Nov 12, 2025

Nice, thanks!

Interesting way to detect an array! However, IIUC this would work for other tuple-types as well, no? Like std::pair<int, std::string>. That would produce very strange compiler errors for sure... But then again, the API does also allow passing a std::vector<uint32_t>, which is just as awful I guess. You could look into using the statically_spanable_range<> concept to detect the array instead.

Anyway: Could you please add a few tests that serialize a BigInt into an array? src/tests/test_bigint.cpp has a function test_bigint_serialization, please add another CHECK that uses std::array. Also check that the exception is thrown as expected if the array is too short or too long.

You're right that it will provide general support for tuple types. I've made the changes as you suggested.

Additionally, for the situation you mentioned, the API does also allow passing a std::vector<uint32_t>, which is just as awful I guess. I think we could use requires std::same_as<typename T::value_type, uint8_t. There were no issues during compilation and testing.

However, since this is a general change to the API, I haven't added it to the branch. What do you think?

Update: 20.01.2026 - Rebase master branch and remove static_cast usage in big_int test code.

@KaganCanSit KaganCanSit requested a review from reneme November 13, 2025 04:53
@KaganCanSit KaganCanSit force-pushed the bigint-array-serialization branch 2 times, most recently from f0e12e3 to fc8b8cd Compare January 21, 2026 04:12
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for changing my mind on this, but I think there's a better strategy to implement this TODO.

@KaganCanSit KaganCanSit force-pushed the bigint-array-serialization branch from fc8b8cd to 6161f70 Compare January 21, 2026 19:24
@KaganCanSit
Copy link
Contributor Author

Sorry for changing my mind on this, but I think there's a better strategy to implement this TODO.

Thanks for the detailed feedback and this learning opportunity.

I've pushed the changes but since I worked on this after hours, I'd like to revisit and review them more thoroughly when I'm fresher (I'm not yet comfortable using concepts actively). I want to make sure I fully understand the reasoning behind each change. I will write experimental code blocks for exploration and possibly add more tests later.

But absolutily like this way, more readable and effective.

@KaganCanSit KaganCanSit force-pushed the bigint-array-serialization branch 2 times, most recently from 5e6e674 to 74c7a7a Compare January 22, 2026 04:04
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @randombit I'm guessing you agree with adding a BigInt::serialize_to_array<32>() given that you left the TODO?

@KaganCanSit
Copy link
Contributor Author

Hit #5376, #5328, #5334, #5341... Rebased master branch.

@KaganCanSit KaganCanSit force-pushed the bigint-array-serialization branch from 1329eff to 73d954c Compare February 24, 2026 18:12
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.

3 participants