-
Notifications
You must be signed in to change notification settings - Fork 11
fix: use Option<CryptoHash> for Transaction block_hash #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,13 +12,36 @@ use crate::{ | |||||||||||||
| AccountId, Action, CryptoHash, Nonce, PublicKey, Signature, errors::DataConversionError, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| /// Borsh-serialize an `Option<CryptoHash>` as a plain `CryptoHash`, preserving | ||||||||||||||
| /// the on-chain wire format. Panics if the value is `None`, since serialization | ||||||||||||||
| /// is only valid for fully-constructed transactions (i.e. those with a known | ||||||||||||||
| /// block hash). | ||||||||||||||
| fn borsh_ser_optional_hash<W: std::io::Write>( | ||||||||||||||
| val: &Option<CryptoHash>, | ||||||||||||||
| writer: &mut W, | ||||||||||||||
| ) -> Result<(), std::io::Error> { | ||||||||||||||
| let hash = val.expect("cannot borsh-serialize a Transaction whose block_hash is None (this transaction was deserialized from an RPC response that lacks block hash information)"); | ||||||||||||||
| BorshSerialize::serialize(&hash, writer) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Borsh-deserialize a plain `CryptoHash` into `Some(CryptoHash)`. | ||||||||||||||
| fn borsh_de_optional_hash<R: std::io::Read>( | ||||||||||||||
| reader: &mut R, | ||||||||||||||
| ) -> Result<Option<CryptoHash>, std::io::Error> { | ||||||||||||||
| CryptoHash::deserialize_reader(reader).map(Some) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] | ||||||||||||||
| pub struct TransactionV0 { | ||||||||||||||
| pub signer_id: AccountId, | ||||||||||||||
| pub public_key: PublicKey, | ||||||||||||||
| pub nonce: Nonce, | ||||||||||||||
| pub receiver_id: AccountId, | ||||||||||||||
| pub block_hash: CryptoHash, | ||||||||||||||
| #[borsh( | ||||||||||||||
| serialize_with = "borsh_ser_optional_hash", | ||||||||||||||
| deserialize_with = "borsh_de_optional_hash" | ||||||||||||||
| )] | ||||||||||||||
| pub block_hash: Option<CryptoHash>, | ||||||||||||||
| pub actions: Vec<Action>, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -28,7 +51,11 @@ pub struct TransactionV1 { | |||||||||||||
| pub public_key: PublicKey, | ||||||||||||||
| pub nonce: Nonce, | ||||||||||||||
| pub receiver_id: AccountId, | ||||||||||||||
| pub block_hash: CryptoHash, | ||||||||||||||
| #[borsh( | ||||||||||||||
| serialize_with = "borsh_ser_optional_hash", | ||||||||||||||
| deserialize_with = "borsh_de_optional_hash" | ||||||||||||||
| )] | ||||||||||||||
| pub block_hash: Option<CryptoHash>, | ||||||||||||||
| pub actions: Vec<Action>, | ||||||||||||||
| pub priority_fee: u64, | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -68,6 +95,13 @@ impl Transaction { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| pub const fn block_hash(&self) -> Option<CryptoHash> { | ||||||||||||||
| match self { | ||||||||||||||
| Self::V0(tx) => tx.block_hash, | ||||||||||||||
| Self::V1(tx) => tx.block_hash, | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| pub fn actions(&self) -> &[Action] { | ||||||||||||||
| match self { | ||||||||||||||
| Self::V0(tx) => &tx.actions, | ||||||||||||||
|
|
@@ -134,13 +168,18 @@ impl TryFrom<near_openapi_types::SignedTransactionView> for SignedTransaction { | |||||||||||||
| signature, | ||||||||||||||
| } = value; | ||||||||||||||
|
|
||||||||||||||
| // The RPC response provides the transaction hash but not the block hash | ||||||||||||||
| // that was used when signing. We store the real tx hash and set block_hash | ||||||||||||||
| // to None since it is unavailable. | ||||||||||||||
| let tx_hash: CryptoHash = hash.into(); | ||||||||||||||
|
|
||||||||||||||
| let transaction = if priority_fee > 0 { | ||||||||||||||
| Transaction::V1(TransactionV1 { | ||||||||||||||
| signer_id, | ||||||||||||||
| public_key: public_key.try_into()?, | ||||||||||||||
| nonce, | ||||||||||||||
| receiver_id, | ||||||||||||||
| block_hash: hash.into(), | ||||||||||||||
| block_hash: None, | ||||||||||||||
| actions: actions | ||||||||||||||
| .into_iter() | ||||||||||||||
| .map(Action::try_from) | ||||||||||||||
|
|
@@ -153,15 +192,19 @@ impl TryFrom<near_openapi_types::SignedTransactionView> for SignedTransaction { | |||||||||||||
| public_key: public_key.try_into()?, | ||||||||||||||
| nonce, | ||||||||||||||
| receiver_id, | ||||||||||||||
| block_hash: hash.into(), | ||||||||||||||
| block_hash: None, | ||||||||||||||
| actions: actions | ||||||||||||||
| .into_iter() | ||||||||||||||
| .map(Action::try_from) | ||||||||||||||
| .collect::<Result<Vec<_>, _>>()?, | ||||||||||||||
| }) | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| Ok(Self::new(Signature::from_str(&signature)?, transaction)) | ||||||||||||||
| let signed = Self::new(Signature::from_str(&signature)?, transaction); | ||||||||||||||
| // Pre-populate with the correct hash from the RPC response, | ||||||||||||||
| // since we cannot recompute it without the block hash. | ||||||||||||||
| let _ = signed.hash.set(tx_hash); | ||||||||||||||
|
||||||||||||||
| let _ = signed.hash.set(tx_hash); | |
| #[allow(clippy::expect_used)] | |
| signed | |
| .hash | |
| .set(tx_hash) | |
| .expect("SignedTransaction hash OnceLock unexpectedly initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
borsh_ser_optional_hashcurrently callsok_or_elsedirectly on&Option<CryptoHash>, which works becauseOption<CryptoHash>isCopytoday, but is a bit non-obvious and introduces an unnecessary copy. Usingas_ref()/as_deref()(or amatch) to get a borrowed hash before serializing would make the intent clearer and avoid relying onCopysemantics.