Add const constructor to StarkFelt

[giladchase]

• Implement from<128> using const logic, so that it can be used as a const constructor.
• Also remove StarkFelt::one() which in favor of StarkFelt::from_128(1_u128).

---

This change is 

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)

a discussion (no related file):
why remove the ::one?
can it be implemented in const way?
I don't like the hard-coded 1_u128

---

[giladchase]

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

a discussion (no related file):
Some context: Basically this whole thing is about creating const StarkFelt instances for small values, which wasn't possible because StarkFelt::from isn't a const function.
::one was added because we didn't have const constructor add someone wanted something equivalent to:

pub const BLOCK_HASH_TABLE_ADDRESS = StarkFelt(1); // note that it is hard coded here as well, but it's a constant.

Back to your questions now :)

why remove the ::one?

We can leave it, it just became a special case of from_128 now so I thought it might be unnecessary.

can it be implemented in const way?

I don't quite follow, both ::one and from_128 are consts

I don't like the hard-coded 1_u128

Isn't it okay if it's a constant though?

---

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul, @giladchase, and @yair-starkware)

a discussion (no related file):

Previously, giladchase wrote…

Some context: Basically this whole thing is about creating const StarkFelt instances for small values, which wasn't possible because StarkFelt::from isn't a const function.
::one was added because we didn't have const constructor add someone wanted something equivalent to:

pub const BLOCK_HASH_TABLE_ADDRESS = StarkFelt(1); // note that it is hard coded here as well, but it's a constant.

Back to your questions now :)

why remove the ::one?

We can leave it, it just became a special case of from_128 now so I thought it might be unnecessary.

can it be implemented in const way?

I don't quite follow, both ::one and from_128 are consts

I don't like the hard-coded 1_u128

Isn't it okay if it's a constant though?

The reason I don't like the 1 is because, when you use numeric constants, it's not always clear if the value is arbitrary or not; ::one is more explicit

---

---

src/hash.rs line 67 at r2 (raw file):

            0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1,
        ])
    }

how bout this?

Suggestion:

    pub const fn one() -> Self {
        Self::from_u128(1_u128)
    }

[giladchase]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @yair-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

The reason I don't like the 1 is because, when you use numeric constants, it's not always clear if the value is arbitrary or not; ::one is more explicit

Fair enough, removed the deprecate TODO as well as putting back the one.

---

---

src/hash.rs line 67 at r2 (raw file):

Previously, dorimedini-starkware wrote…

how bout this?

Right again :)

[yair-starkware]

What about the other types (TransactionHash, ContractAddress, TransactionVersion, etc.)?
Can we do the same for them?
In our code we use a lot of lazy_static for this

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @giladchase)

---

src/hash.rs line 75 at r3 (raw file):

        }
        Self(bytes)
    }

Why is this code changed?
IIUC before it was

        let mut bytes = [0u8; 32];
        bytes[16..32].copy_from_slice(&val.to_be_bytes());
        Self(bytes)

Also, why not

for index in 16..32

Code quote:

    pub const fn from_u128(val: u128) -> Self {
        let mut bytes = [0u8; 32];
        let val_bytes = val.to_be_bytes();
        let mut index = 16;
        while index < 32 {
            bytes[index] = val_bytes[index - 16];
            index += 1;
        }
        Self(bytes)
    }

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)

[giladchase]

For TransactionHashandTransactionVersion` it isn't strictly necessary, because now you can do:

pub const ZERO: TransactionVersion = TransactionVersion(StarkFelt::from_u128(0_u128));

So adding a const constructor for them would just be sugar.

For ContractAddress we might want to add one though (in a separate PR), because it'll require adding a const constructor for PatriciaKey.

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

---

src/hash.rs line 75 at r3 (raw file):

Why is this code changed?

It's doing the same thing the const constructor does, only with non-const methods (like copy_from_slice), so this removed the duplication.
Also using the const implementation will be faster because it is computed at compile-time.

Also, why not

for isn't allowed in const functions.
You can use control-flow in const, but only a very limited subset of them.

[yair-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)

[yair-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)