Granola-Team / mina-block-explorer

Web application for the Mina blockchain
Apache License 2.0
6 stars 5 forks source link

fix: limit from i64 -> u64 #835

Closed jhult closed 23 hours ago

jhult commented 1 month ago

u16 has a range from 0 to 65,535. This should be plenty for a limit.

Fixes #600

robinbb commented 1 month ago

I manually canceled the build.

robinbb commented 1 month ago

Actually, we may not want to limit the limit to merely 16 bits, on some of these. There may be, in the future, some sort of limit on the limits, but we probably don't want our code to be limited by bits.

trevorbernard commented 1 month ago

u16 is too small. There are more accounts than the size of u16. Really ulimit should be usize

jhult commented 1 month ago

u16 is too small. There are more accounts than the size of u16. Really ulimit should be usize

There are more accounts than u16 (65,535). However, for a limit on querying these, u16 is already more than the (1000) we support...

robinbb commented 1 month ago

I'm saying that while there should be a limit, that limit should be specified as a constant somewhere (probably compile time, but could even be runtime), and I wouldn't want that to be limited to 2^16. Only the front-end needs a low limit. Recal that the indexer may serve as a back-end for a variety of uses, not merely to satisfy the in-browser front-end.

n1tranquilla commented 4 weeks ago

Much of the underlying graphql generated code assumes i64. That's why you see casting back to i64 in some of the function.rs files. This is somewhat unavoidable, given the limitations of the library.

Merged in main and resolved conflicts.

n1tranquilla commented 3 weeks ago

Re-running the tests since they have stabilized recently.

robinbb commented 3 weeks ago

I think that @jhult intends to close this PR, splitting into 2+ others.

jhult commented 1 day ago

I found the issue. I was using .unwrap_or_default() as i64 and this was defaulting to 0 in some cases instead of what I actually wanted (None). Now I'm using .map(|x| x as i64).

robinbb commented 23 hours ago

@jhult Best to make CI go green before merging.