starkware-libs / starknet-api Goto Github PK
View Code? Open in Web Editor NEWLicense: Apache License 2.0
License: Apache License 2.0
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
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.
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
starknet-api/src/transaction.rs
Line 489 in a50e621
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.
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
The FunctionAbiEntry
struct is missing the stateMutability
field from the compiled .json
.
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.
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
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.
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
#[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
For example, serializing ChainID
should return a hex encoded string rather than a plain string
/// 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.
Gm, do you have any plans on making Python SDK for your API?
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
debug_info
should be optional.
https://github.com/starkware-libs/starknet-api/blob/main/src/deprecated_contract_class.rs#L93
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
#[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.
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#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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.