feat(starknet_api): remove Ord and PartialOrd from TransactionVersion, implement explicit cmp

[dorimedini-starkware]

…, implement explicit cmp functions

[reviewable-StarkWare]

This change is 

[dorimedini-starkware]

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @noaov1)

a discussion (no related file):
Also removed many Ord, PartialOrd derives; the objects they were derived on don't seem to have a natural order. Unclear why these derives were needed...

---

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @noaov1)

---

crates/starknet_api/src/transaction.rs line 862 at r1 (raw file):

    pub fn base_lt(&self, other: &Self) -> bool {
        self.without_query_bit().0 < other.without_query_bit().0

I can do self.base_le(other) && self.without_query_bit() != other.without_query_bit() but that would be 4 conversions instead of 2

Code quote:

self.without_query_bit().0 < other.without_query_bit().0

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.39%. Comparing base (e3165c4) to head (a242b5c).
Report is 56 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_api/src/transaction.rs	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1638       +/-   ##
===========================================
+ Coverage   40.10%   73.39%   +33.29%     
===========================================
  Files          26      163      +137     
  Lines        1895    21889    +19994     
  Branches     1895    21889    +19994     
===========================================
+ Hits          760    16066    +15306     
- Misses       1100     4611     +3511     
- Partials       35     1212     +1177     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ArniStarkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[giladchase]

Note: this is a breaking change that might disrupt users who relied on the partialord behavior (like we did).
Can you involve product.

If this gets the OK, I still think it should be separated into two PRs, one removing unused derived and removing used derives with a replacement that explains why that order was chosen.

Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

Also removed many Ord, PartialOrd derives; the objects they were derived on don't seem to have a natural order. Unclear why these derives were needed...

Orphan rule + legacy, if we don't implement PartialOrd/Ord then no-one can (except via newtypes), somebody probably added these all over SNAPI without thinking too much on whether or not it makes sense.
But you are right about the natural order.

---

---

crates/starknet_api/src/transaction.rs line 854 at r2 (raw file):

        let mut self_biguint = self.0.to_biguint();
        self_biguint.set_bit(QUERY_VERSION_BASE_BIT.into(), false);
        Self(self_biguint.into())

Seems to expensive for cmp, biguint is a Vec, can this done on the stack?
Also can you explain what base here means and about the query_version_base_bit?

Code quote:

        let mut self_biguint = self.0.to_biguint();
        self_biguint.set_bit(QUERY_VERSION_BASE_BIT.into(), false);
        Self(self_biguint.into())

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)

---

crates/starknet_api/src/transaction.rs line 862 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I can do self.base_le(other) && self.without_query_bit() != other.without_query_bit() but that would be 4 conversions instead of 2

Conversions aren't the issue, this allocates two vectors on the heap just to compare two fels

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1)

a discussion (no related file):
Closing this, and will implement a "natural" order on TransactionVersion (ZERO < ZERO_QUERY < ONE < ONE_QUERY ...)

---