starkware-libs / starknet-api Goto Github PK

debug_info should be optional.
https://github.com/starkware-libs/starknet-api/blob/main/src/deprecated_contract_class.rs#L93

#[derive(
    Clone,
    Copy,
    Debug,
    Default,
    Deserialize,
    Eq,
    Hash,
    Ord,
    PartialEq,
    PartialOrd,
    Serialize,
    derive_more::Deref,
)]
#[cfg_attr(
    feature = "parity-scale-codec",
    derive(parity_scale_codec::Encode, parity_scale_codec::Decode)
)]
#[cfg_attr(feature = "scale-info", derive(scale_info::TypeInfo))]
#[serde(from = "PrefixedBytesAsHex<8_usize>", into = "PrefixedBytesAsHex<8_usize>")]
pub struct Tip(pub u64);

As mention in #197 don't use deref. Use From/Into instead

https://github.com/starkware-libs/starknet-api/blob/main/src/core.rs#L20-L21

here ChainId is represented as a String

But in the specs the ChainId is used to compute the tx hash of all the transactions. This means it should fit in a Felt meaning it should be at most a ShortString of max size 31 characters.

I recommend we store it as a Felt for its internal representation, as it will simplify its usage for tx hash computation, and implement Display on it in a way that returns the short string representation.

With the current inner repr as String and no new method to make sure the max length is respected, anybody can write code that will break the protocol.

This trait should exist and be implemented on each transaction type:

pub trait ComputeTransactionHash {
    fn compute_hash<H: HasherT>(&self, chain_id: Felt, is_query: bool) -> TransactionHash;
}

The problem is that in order to compute the hash of a transaction, we need to know its version. And with DeclareTransactionV0V1 we can't know what is the actual version without some external additional data, it could be 0 it could be 1, making it impossible to use the same trait as for the other transaction types.

I propose this type is removed, and split into two different structs.

Also, correct me if I'm wrong, but DeclareV0 has no nonce, so there is no reason to represent it with a type that contains a nonce field

        let account_tx_context = match transaction {
            Transaction::Declare(tx) => AccountTransactionContext {
                transaction_hash: tx.transaction_hash(),
                max_fee: tx.max_fee(),
                version: tx.version(),
                signature: tx.signature(),
                nonce: tx.nonce(),
                sender_address: tx.sender_address(),
            },
            Transaction::Deploy(tx) => panic!("Legacy code in starknet-api."),
            Transaction::DeployAccount(tx) => AccountTransactionContext {
                transaction_hash: tx.transaction_hash,
                max_fee: tx.max_fee,
                version: tx.version,
                signature: tx.signature,
                nonce: tx.nonce,
                sender_address: tx.contract_address,
            },
            Transaction::Invoke(tx) => AccountTransactionContext {
                transaction_hash: tx.transaction_hash(),
                max_fee: tx.max_fee(),
                version: TransactionVersion(StarkFelt::from(0_u8)),
                signature: tx.signature(),
                nonce: tx.nonce(),
                sender_address: tx.sender_address(),
            },
            Transaction::L1Handler(tx) => AccountTransactionContext {
                transaction_hash: tx.transaction_hash,
                max_fee: Default::default(),
                version: tx.version,
                signature: Default::default(),
                nonce: tx.nonce,
                sender_address: tx.sender_address,
            },
        };

Right now the getters functions are only implemented for Invoke and Declare, no for the other types of tx. It is inconsistent.
Ideally it should be implement through a trait. At least for those that are commons: tx_hash, nonce, version. The other could return an None when not relevant.
It could also be impl for the Transaction enum itself

Gm, do you have any plans on making Python SDK for your API?

macro_rules! implement_deploy_account_tx_getters {
    ($(($field:ident, $field_type:ty)),*) => {
        $(
            pub fn $field(&self) -> $field_type {
                match self {
                    Self::V1(tx) => tx.$field.clone(),
                    Self::V3(tx) => tx.$field.clone(),
                }
            }
        )*
    };
}

...

impl DeployAccountTransaction {
    implement_deploy_account_tx_getters!(
        (class_hash, ClassHash),
        (constructor_calldata, Calldata),
        (contract_address_salt, ContractAddressSalt),
        (nonce, Nonce),
        (signature, TransactionSignature)
    );

Calling clone is not a problem for types that implement copy, but for types that don't. Like TransactionSignature, it means that a clone operation always occurs, even when the caller has no need for ownership of the value and could have used a reference.
This has a performance cost and is overall a bad design for an API: the method should not assume anything about the caller intentions

The FunctionAbiEntry struct is missing the stateMutability field from the compiled .json.

https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/transactions/#v3_hash_calculation

The doc states that while computing the hash of a tx v3 the DataAvailabilityMode should be represented as a u32 and concatenated inside a Felt.

It would be handy to have this method coming with the lib.
Right now I have to do

nonce_data_availability_mode as u32

This implies a silent overflow if the DataAvailabilityMode internal happens to be greater than a u32.
I know it is not the case, but, as a lib user, I shouldn't have to ask the question and go check the code myself.

If I want to create a transaction right now here is the flow:

    let mut tx = DeclareTransactionV0V1 {
        max_fee,
        signature: Default::default(),
        nonce,
        class_hash,
        sender_address,
    };
    let tx_hash = tx.compute_hash();
    tx.signature = sign_tx_hash(tx_hash);

This flow feels really bad because I start by building a tx with a dummy value for signature, use it, and then later replace this dummy value with the actual one.
I also have to create my tx as mutable.

Overall this introduces a lot of room for errors because the same type is actually used to represent two different types: Signed and NonSigned transactions. And for one of those, it adds a non-used field.
A good way to solve this would be the introduction of a SignedTransaction type:

struct SignedTransaction<T: StarknetTransaction> {
  tx: T,
  signature: TransactionSignature,
}

and maybe a trait

trait TransactionSigner<T: StarknetTransaction> {
  fn sign_transaction(transaction: T) -> SignedTransaction<T>;
}

It would then become really easy for the user to implement some signature scheme, key management, etc in a way that is not error-proof and feels good to use.

The transaction version is not part of the starknet state. It is occasionally used as an Felt to compute some hash, but most of the time it is manipulated by the sequencer in a non-provable context, so there is no reason for it to have Felt being its default representation.
It should be an u8, and a Felt can easily be constructed from an u8 when needed.

It's a really usual thing to print a transaction hash as a hex string. Rust provides a specific trait to realize this operation We should use it.
There is most likely other types that need to implement it in this lib

Blockifier transaction types are build ontop of starknet API ones and require you to provide the transaction hash.
The tx hash can be computed based on the tx if you add the chain_id and boolean signaling if you need to offset the version number. You also can make it generic over a hashing function.

This logic should be implemented as a trait on every transaction type

trait ComputeTransactionHash {
  fn compute_hash<H: HasherT>(&self, chain_id: Starkfelt, offset_version: bool) -> TransactionHash;
}

This would avoid every lib consumer to reimplement this logic themselves everytime

ResourceBoundsMapping only contains two values for now, L1Gas and L2Gas.

First of all, why is it represented as a BTreeMap? It could just be a struct with two ResourceBounds fields. Is it to be future-proof? That's an unnecessary worry and it comes with performance cost.

Even if we keep it like this, could we at least implement some getters function on ResourceBoundsMapping:

fn l1_gas(&self) -> &ResourceBounds;
fn l2_gas(&self) -> &ResourceBounds;

It can also return an Option<ResourceBounds> if a tx without one or both values is deemed valid.

I would avoid having to write this

self.resource_bound.0.get(resource)

https://github.com/starkware-libs/starknet-api/blob/main/src/core.rs#L48-L50

The Deref traits have been derived over many of the wrapper types built on top of the StarkFetl type.
I guess that it was done to reduce the boilerplate induced by this design, which resulted in some "URL-like" syntax, eg: "contract_address.0.0".

This a a major design issue. Deref should be used for smart pointers and similar types. It is a feature of the language which is built around the concept of References, it should not be used for regular types casting. Into/From is the language intended way to do this type of operation. Those are the traits you should be implementing on those many wrapper types.

The Deref trait is a powerful tool, that is used in many places and should be implemented with care and only for smart pointers. You will find may resources online explaining why this specific usage is wrong, but you can start with this StackOverflow question. The Deref doc is also a good source of knowledge on this question, but a bit less explicit.

What should be done?
Those derive implementations should be removed as soon as possible.
They can later on be replaced by some From/Into implementations, that you can also get from the derive_more crate.

#[derive(
    Clone, Copy, Debug, Deserialize, EnumIter, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize,
)]
#[cfg_attr(
    feature = "parity-scale-codec",
    derive(parity_scale_codec::Encode, parity_scale_codec::Decode)
)]
#[cfg_attr(feature = "scale-info", derive(scale_info::TypeInfo))]
pub enum Resource {
    #[serde(rename = "L1_GAS")]
    L1Gas,
    #[serde(rename = "L2_GAS")]
    L2Gas,
}

The string "L1_GAS" and "L2_GAS" are defined by the protocol. Right now I have to define those myself while using this lib:

const L1_GAS: &[u8] = b"L1_GAS";
const L2_GAS: &[u8] = b"L2_GAS";

...

match resource {
        Resource::L1Gas => L1_GAS,
        Resource::L2Gas => L2_GAS,
    } 

This is error prove and unnecessary. starknet-api should provide a way for the lib consumers to access those values directly.
Display provides the to_string method, which seems like a good way to implement this. It could also be used in your serde(rename) tag, instead of hardcoding it.

https://doc.rust-lang.org/1.26.2/unstable-book/language-features/repr-transparent.html

This would make it safe to do some unsafe memory operation on those types + other nice things

/// The hash of a ContractClass.
#[derive(
    Debug,
    Default,
    Copy,
    Clone,
    Eq,
    PartialEq,
    Hash,
    Deserialize,
    Serialize,
    PartialOrd,
    Ord,
    Display,
)]
#[cfg_attr(
    feature = "parity-scale-codec",
    derive(parity_scale_codec::Encode, parity_scale_codec::Decode)
)]
#[cfg_attr(feature = "scale-info", derive(scale_info::TypeInfo))]
pub struct ClassHash(pub StarkHash);

/// The hash of a compiled ContractClass.
#[derive(
    Debug,
    Default,
    Copy,
    Clone,
    Eq,
    PartialEq,
    Hash,
    Deserialize,
    Serialize,
    PartialOrd,
    Ord,
    Display,
)]
#[cfg_attr(
    feature = "parity-scale-codec",
    derive(parity_scale_codec::Encode, parity_scale_codec::Decode)
)]
#[cfg_attr(feature = "scale-info", derive(scale_info::TypeInfo))]
pub struct CompiledClassHash(pub StarkHash);

So we have two really similar struct here.
We are using two types of class hashes in Starknet, the hash of the contract compiled to sierra and the one of the contract compiled to casm.

So my guess is that ClassHash is SierraClassHash and CompiledClassHash is for CasmClassHash.
But that could be the opposite. It's misleading because both hashes are the hashes of a compiled object, one to Sierra, and the other to Casm.
It could also be that the first one is the hash of something that is not compiled (in opposition to the second), so is the hash of the .cairo files? Or the hash of the in-memory ContractClass struct?

Would it be possible to shed some light on this? And change in naming to make it explicit for lib users. And some doc.

For example, serializing ChainID should return a hex encoded string rather than a plain string

The rpc method l1_handler_tx_receipt contains a message_hash field:
https://github.com/starkware-libs/starknet-specs/blob/76bdde23c7dae370a3340e40f7ca2ef2520e75b9/api/starknet_api_openrpc.json#L3072

Therefore it seems to be part of the protocol. Maybe a method should be available for the users of this lib to easily compute it, rather than having everyone reimplement it manually.

Here is pathfinder impl:
https://github.com/eqlabs/pathfinder/blob/03f8a58c40c69e60ad29ac6e97d7782aeccad4b4/crates/common/src/transaction.rs#L435