search

helium / oracles

Oracles for Helium subDAOs
Apache License 2.0
17 stars 18 forks source link

Refactor: Hex location use `hextree::Cell` type #797

Closed michaeldjeffrey closed 2 months ago

michaeldjeffrey commented 2 months ago

This is something that I was planning on doing as part of HIP-109 changes I was going to be making. So I thought I'd push it early to make it easier to review.

Got some advice from Jay to only use a h3o::CellIndex when you need to convert to/from a LatLng. So we went with hextree::Cell for the type.

hextree::Cell is a loose wrapper around a u64. Parsing the value as early as possible tightens the gaurantee that we can make valid types that need use Cells.

It is also an attempt to remove discrepancies where some types carry a i64 hex straight from a database, and other types carry a u64 that were converted out of a database, even though they're goal is the same.

(see previous commit where #[sqlx(try_from = "i64")] was enabled by the update to hextree).

michaeldjeffrey commented 2 months ago

The only point I would make is that maybe it doesnt go wide enough. We now have a location for the purposes of boosted hexes defined as a Cell but in all other uses of location we still have it as a u64. This inconsistency niggles me.

That's where I'd like to be. Considering all this change fanned out from the BoostedHex::location field, it seemed more responsible to switch over only the things related to what I plan on touching and get that change in. Rather than starting from the source and me having to make decisions for parts of the code I haven't interacted with yet, and landing a behemoth of a refactor PR.