feat: impl ToBigInt and ToBigUInt

[tdelabro]

No description provided.

[0xLucqs]

tests

[pefontana]

Hi @tdelabro !
I dont get why we need this, when we have the Felt methods pub fn to_bigint(&self) -> BigInt and pub fn to_biguint(&self) -> BigUint

[tdelabro]

@pefontana we need it in order to be compatible with traits that would require the implementation of ToBigInt and ToBigUInt.
The whole num-traits thing is behind a feature flag, but if you need it to interface with some libs methods or traits, you can enable the flag and use the felt type there.

[0xLucqs]

still waiting for tests

[tdelabro]

still waiting for tests

The conversion logic is already tested there:

types-rs/crates/starknet-types-core/src/felt/mod.rs

Line 1931 in 36f5be1

fn felt_to_bigints() {

There is no logic to be tested in the code added by this PR.

[0xLucqs]

for the current implementation yes but if we change something we'll get rekt, just copy paste the other tests idk

[tdelabro]

for the current implementation yes but if we change something we'll get rekt, just copy paste the other tests idk

It doesn't make sense to test twice the same thing for a change that may or may not occur in the feature.
Let's add the tests when we change the impl, if it ever happens

[tdelabro]

@Oppen can we merge this?

[Oppen]

LGTM

[Oppen]

@Oppen can we merge this?

LGTM but I'm not blocking the merge. Also I don't think I have merge rights anymore, official maintainers are now only Levy and Peter AFAIK.

That said, I do see a point in the tests Lucas asked for, mostly to detect regressions. Even if the tests only assert that the two ways to convert give the same results that's some assurance for future us. I do disagree that the difference it makes here is enough to block merging tho.

stale