feat(transaction): implement rpc transaction

[MohammadNassar1]

This change is 

[ayeletstarkware]

Reviewed 3 of 4 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

src/rpc_transaction.rs line 37 at r2 (raw file):

/// [`Starknet specs`]: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_api_openrpc.json
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
#[serde(deny_unknown_fields)]

Is this necessary? it's not in ExternalTransaction

---

src/rpc_transaction.rs line 50 at r2 (raw file):

///
/// [`Starknet specs`]: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_api_openrpc.json
#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]

Where do you use Hash, Ord and PartialOrd? (also for the other tx type)

---

src/rpc_transaction.rs line 72 at r2 (raw file):

/// A declare transaction of a Cairo-v1 contract class that can be added to Starknet through the
/// RPC.
#[derive(Debug, Deserialize, Serialize, Clone, Eq, PartialEq)]

alphabetize

---

src/rpc_transaction.rs line 92 at r2 (raw file):

/// A deploy account transaction that can be added to Starknet through the RPC.
#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct RPCDeployAccountTransactionV3 {

in ExternalTransaction there's #[serde(deny_unknown_fields)]. why not here?
same for RPCInvokeTransactionV3

---

src/rpc_transaction.rs line 123 at r2 (raw file):

#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]
pub struct ContractClass {
    pub sierra_program: Vec<StarkFelt>,

The code below is from ExternalTransaction. Why did you change it? Because of the spec? I see that this field used to be a String, and now it's a vector of StarkFelts.

Code snippet:

#[serde(rename = "sierra_program")]
 pub compressed_sierra_program: String,

---

src/rpc_transaction.rs line 130 at r2 (raw file):

#[derive(Debug, Clone, Default, Eq, PartialEq, Deserialize, Serialize)]
pub struct EntryPointByType {

Why did you decide to change entry_points_by_type, that was HashMap<EntryPointType, Vec<EntryPoint>> (EntryPointType as an enum) to a struct?

[giladchase]

Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

src/rpc_transaction.rs line 35 at r2 (raw file):

/// either a contract class or a class hash).
///
/// [`Starknet specs`]: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_api_openrpc.json

🤩

Code quote:

/// [`Starknet specs`]: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_api_openrpc.json

---

src/rpc_transaction.rs line 130 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why did you decide to change entry_points_by_type, that was HashMap<EntryPointType, Vec<EntryPoint>> (EntryPointType as an enum) to a struct?

In general, if you have a hash with fixed fields it should be a struct, it is better:

a) you get a compile-time check that all keys you want exist for free
b) easier to access the values, rather than having to do a get(thing).unwrap() or something like that
c) faster access, more optimized

probably other that I forgot.
But basically hashmaps are largly for dynamic content, not fixed.

---

src/rpc_transaction_test.rs line 1 at r2 (raw file):

use std::sync::Arc;

no logic changes right? just move/rename?

[MohammadNassar1]

Reviewable status: 3 of 6 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

---

src/rpc_transaction.rs line 35 at r2 (raw file):

Previously, giladchase wrote…

🤩

:)

---

src/rpc_transaction.rs line 37 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is this necessary? it's not in ExternalTransaction

Moved to ExternalTransaction

---

src/rpc_transaction.rs line 50 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Where do you use Hash, Ord and PartialOrd? (also for the other tx type)

Note that this struct should be shared among other crates.
So, I prefer to keep additional traits that can be useful for others.

---

src/rpc_transaction.rs line 72 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

alphabetize

Done.

---

src/rpc_transaction.rs line 92 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

in ExternalTransaction there's #[serde(deny_unknown_fields)]. why not here?
same for RPCInvokeTransactionV3

Good point.
I think it should be added for ExternalTransaction struct.
As the deserialization is done over ExernalTransaction struct.

---

src/rpc_transaction.rs line 123 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The code below is from ExternalTransaction. Why did you change it? Because of the spec? I see that this field used to be a String, and now it's a vector of StarkFelts.

Correct, changed to be aligned with the spec.

---

src/rpc_transaction_test.rs line 1 at r2 (raw file):

Previously, giladchase wrote…

no logic changes right? just move/rename?

correct

[MohammadNassar1]

Reviewable status: 3 of 6 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

a discussion (no related file):
In this PR, I changed the struct fields to be aligned with the spec.
Next PR, rename to RPCTransaction.

---

[giladchase]

Reviewed 1 of 4 files at r2, 3 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArniStarkware and @ayeletstarkware)

[ayeletstarkware]

Reviewed 3 of 5 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @MohammadNassar1)

---

src/external_transaction.rs line 28 at r3 (raw file):

    DeployAccount(ExternalDeployAccountTransaction),
    /// An invoke transaction.
    #[serde(rename = "INVOKE_FUNCTION")]

why did you remove it?

Code quote:

_FUNCTION

---

src/external_transaction_test.rs line 21 at r3 (raw file):

fn create_resource_bounds() -> ResourceBoundsMapping {
    let mut map = BTreeMap::new();
    map.insert(Resource::L1Gas, ResourceBounds { max_amount: 100, max_price_per_unit: 12 });

What are these values? I see you use default values now.

---

src/executable_transaction.rs line 4 at r1 (raw file):

use crate::state::ContractClass;
use crate::transaction::TransactionHash;

why?

[MohammadNassar1]

Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

---

src/external_transaction.rs line 28 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why did you remove it?

See here

---

src/external_transaction_test.rs line 21 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

What are these values? I see you use default values now.

I've added a new function that generates non-default values.

---

src/executable_transaction.rs line 4 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why?

This file wasn't changed.
It's not related to the external tx.

[ayeletstarkware]

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @MohammadNassar1)

---

src/external_transaction_test.rs line 21 at r3 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I've added a new function that generates non-default values.

Add a comment explaining that these are non-default values because they're random and seem to mean something.

[MohammadNassar1]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @ayeletstarkware)

---

src/external_transaction_test.rs line 21 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Add a comment explaining that these are non-default values because they're random and seem to mean something.

Done.

[ayeletstarkware]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

[ayeletstarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

[ArniStarkware]

src/external_transaction.rs line 122 at r5 (raw file):

#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]
pub struct ContractClass {
    pub sierra_program: Vec<StarkFelt>,

Can we be aligned with the class ContractClass used in the compilers repo?

Suggestion:

    pub sierra_program: Vec<BigUintAsHex>,

[ArniStarkware]

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

---

src/external_transaction.rs line 122 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Can we be aligned with the class ContractClass used in the compilers repo?

Discussed F2F. This is complicated.

[ArniStarkware]

src/external_transaction.rs line 122 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Discussed F2F. This is complicated.

Wait - we talked F2f about how this member needs to be consistent with the RPC.
(look for the line: The list of Sierra instructions of which the program consists in here
But we just changed this value (to fit this better, I admit). Who is in charge of the RPC? Are we sure they are using the same values for external transactions? Can the external transaction use a different ContractClass struct than the one Papyrus is using?

[MohammadNassar1]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

---

src/external_transaction.rs line 122 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Wait - we talked F2f about how this member needs to be consistent with the RPC.
(look for the line: The list of Sierra instructions of which the program consists in here
But we just changed this value (to fit this better, I admit). Who is in charge of the RPC? Are we sure they are using the same values for external transactions? Can the external transaction use a different ContractClass struct than the one Papyrus is using?

Why do you think we use a different value?
What do you mean by external transaction? the one that is different than RPC.

I think Ariel is in charged of this spec.

[ArniStarkware]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @ayeletstarkware and @giladchase)

---

src/external_transaction.rs line 122 at r5 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why do you think we use a different value?
What do you mean by external transaction? the one that is different than RPC.

I think Ariel is in charged of this spec.

NVM.

[ArniStarkware]

Reviewed 1 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware and @giladchase)