Skip to content
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

feat: add serialization for TransactionRequest #471

Merged
merged 9 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## v0.5.0 (TBD)

* Added serialization for `TransactionRequest` (#471).
* Added the Web Client Crate
* Added validations in transaction requests (#447).
* [BREAKING] Refactored `Client` to merge submit_transaction and prove_transaction (#445)
Expand Down
185 changes: 181 additions & 4 deletions crates/rust-client/src/transactions/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ use core::fmt;

use miden_objects::{
accounts::AccountId,
assembly::AssemblyError,
assembly::{AssemblyError, AstSerdeOptions, ProgramAst},
assets::{Asset, FungibleAsset},
crypto::merkle::{InnerNodeInfo, MerkleStore},
notes::{Note, NoteDetails, NoteId, NoteType, PartialNote},
transaction::{OutputNote, TransactionArgs, TransactionScript},
vm::AdviceMap,
Digest, Felt, Word,
};
use miden_tx::utils::{ByteReader, ByteWriter, Deserializable, DeserializationError, Serializable};

// MASM SCRIPTS
// ================================================================================================
Expand Down Expand Up @@ -256,6 +257,132 @@ impl TransactionRequest {
}
}

impl Serializable for TransactionRequest {
fn write_into<W: ByteWriter>(&self, target: &mut W) {
self.account_id.write_into(target);
self.unauthenticated_input_notes.write_into(target);
self.input_notes.write_into(target);
match &self.script_template {
None => target.write_u8(0),
Some(TransactionScriptTemplate::CustomScript(script)) => {
target.write_u8(1);
script.code().write_into(target, AstSerdeOptions { serialize_imports: true });
script.code().write_source_locations(target);
script.hash().write_into(target);
script.inputs().write_into(target);
},
Some(TransactionScriptTemplate::SendNotes(notes)) => {
target.write_u8(2);
notes.write_into(target);
},
}
self.expected_output_notes.write_into(target);
self.expected_future_notes.write_into(target);
self.advice_map.clone().into_iter().collect::<Vec<_>>().write_into(target);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid cloning the advice map here?

Copy link
Collaborator Author

@tomyrd tomyrd Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AdviceMap only implements IntoIter so we have to consume the map to iterate it. In this case the map is a reference which we can't consume. If we don't want to clone it the two alternatives that I can think of would be to:

  • Implement deserialization for AdviceMap so we can use write_into.
  • Implement .iter() to iterate over a reference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both of these would need to be done in miden-vm - right? If so, let's keep it like it is now, add a TODO here and create an issue for this in the miden-vm repo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.merkle_store.write_into(target);
}
}

impl Deserializable for TransactionRequest {
fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> {
let account_id = AccountId::read_from(source)?;
let unauthenticated_input_notes = Vec::<Note>::read_from(source)?;
let input_notes = BTreeMap::<NoteId, Option<NoteArgs>>::read_from(source)?;

let script_template = match source.read_u8()? {
0 => None,
1 => {
let mut code = ProgramAst::read_from(source)?;
code.load_source_locations(source)?;
let hash = Digest::read_from(source)?;
let inputs = BTreeMap::<Digest, Vec<Felt>>::read_from(source)?;

let transaction_script = TransactionScript::from_parts(
code,
hash,
inputs.into_iter().map(|(k, v)| (k.into(), v)),
)
.map_err(|err| {
DeserializationError::UnknownError(format!(
"Invalid transaction script: {}",
err
))
})?;

Some(TransactionScriptTemplate::CustomScript(transaction_script))
},
2 => {
let notes = Vec::<PartialNote>::read_from(source)?;
Some(TransactionScriptTemplate::SendNotes(notes))
},
_ => {
return Err(DeserializationError::InvalidValue(
"Invalid script template type".to_string(),
))
},
};

let expected_output_notes = BTreeMap::<NoteId, Note>::read_from(source)?;
let expected_future_notes = BTreeMap::<NoteId, NoteDetails>::read_from(source)?;

let mut advice_map = AdviceMap::new();
let advice_vec = Vec::<(Digest, Vec<Felt>)>::read_from(source)?;
advice_map.extend(advice_vec);
let merkle_store = MerkleStore::read_from(source)?;

Ok(TransactionRequest {
account_id,
unauthenticated_input_notes,
input_notes,
script_template,
expected_output_notes,
expected_future_notes,
advice_map,
merkle_store,
})
}
}

impl PartialEq for TransactionRequest {
fn eq(&self, other: &Self) -> bool {
let same_advice_map_count = self.advice_map.clone().into_iter().count()
== other.advice_map.clone().into_iter().count();
let same_advice_map = same_advice_map_count
&& self
.advice_map
.clone()
.into_iter()
.all(|elem| other.advice_map.get(&elem.0).map_or(false, |v| v == elem.1));

let same_script_template = match &self.script_template {
Some(TransactionScriptTemplate::CustomScript(script)) => match &other.script_template {
Some(TransactionScriptTemplate::CustomScript(other_script)) => {
// TODO: We should also compare the source code. However, the ProgramAst
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should warn!("Checking equality for TransactionRequests does not involve comparing the source code") here so that it does not go unnoticed until the TODO is addressed.

// serialization has a small bug that makes the end program a bit different.
// We should add the comparison of the source code once we start using the
// v0.10 `miden-vm`.
script.hash() == other_script.hash() && script.inputs() == other_script.inputs()
},
_ => false,
},
Some(TransactionScriptTemplate::SendNotes(notes)) => match &other.script_template {
Some(TransactionScriptTemplate::SendNotes(other_notes)) => notes == other_notes,
_ => false,
},
None => other.script_template.is_none(),
};

self.account_id == other.account_id
&& self.unauthenticated_input_notes == other.unauthenticated_input_notes
&& self.input_notes == other.input_notes
&& same_script_template
&& self.expected_output_notes == other.expected_output_notes
&& self.expected_future_notes == other.expected_future_notes
&& same_advice_map
&& self.merkle_store == other.merkle_store
}
}

// TRANSACTION REQUEST ERROR
// ================================================================================================

Expand Down Expand Up @@ -419,21 +546,25 @@ pub mod known_script_roots {
#[cfg(test)]
mod tests {
use alloc::string::ToString;
use std::vec::Vec;

use miden_lib::notes::{create_p2id_note, create_p2idr_note, create_swap_note};
use miden_objects::{
accounts::{
account_id::testing::{
ACCOUNT_ID_FUNGIBLE_FAUCET_OFF_CHAIN, ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN,
},
AccountId,
AccountId, AccountType,
},
assets::FungibleAsset,
crypto::rand::RpoRandomCoin,
crypto::rand::{FeltRng, RpoRandomCoin},
notes::NoteType,
Felt, FieldElement,
transaction::OutputNote,
Digest, Felt, FieldElement, ZERO,
};
use miden_tx::utils::{Deserializable, Serializable};

use super::TransactionRequest;
use crate::transactions::known_script_roots::{P2ID, P2IDR, SWAP};

// We need to make sure the script roots we use for filters are in line with the note scripts
Expand Down Expand Up @@ -479,4 +610,50 @@ mod tests {
assert_eq!(p2idr_note.script().hash().to_string(), P2IDR);
assert_eq!(swap_note.script().hash().to_string(), SWAP);
}

#[test]
fn transaction_request_serialization() {
let sender_id = AccountId::new_dummy([0u8; 32], AccountType::RegularAccountImmutableCode);
let target_id = AccountId::new_dummy([1u8; 32], AccountType::RegularAccountImmutableCode);
let faucet_id = AccountId::new_dummy([2u8; 32], AccountType::FungibleFaucet);
let mut rng = RpoRandomCoin::new(Default::default());

let mut notes = vec![];
for i in 0..6 {
let note = create_p2id_note(
sender_id,
target_id,
vec![FungibleAsset::new(faucet_id, 100 + i).unwrap().into()],
NoteType::Private,
ZERO,
&mut rng,
)
.unwrap();
notes.push(note);
}

let mut advice_vec: Vec<(Digest, Vec<Felt>)> = vec![];
for i in 0..10 {
advice_vec.push((Digest::new(rng.draw_word()), vec![Felt::new(i)]));
}

// This transaction request wouldn't be valid in a real scenario, it's intended for testing
let tx_request = TransactionRequest::new(sender_id)
.with_authenticated_input_notes(vec![(notes.pop().unwrap().id(), None)])
.with_unauthenticated_input_notes(vec![(notes.pop().unwrap(), None)])
.with_expected_output_notes(vec![notes.pop().unwrap()])
.with_expected_future_notes(vec![notes.pop().unwrap().into()])
.extend_advice_map(advice_vec)
.with_own_output_notes(vec![
OutputNote::Full(notes.pop().unwrap()),
OutputNote::Partial(notes.pop().unwrap().into()),
])
.unwrap();

let mut buffer = Vec::new();
tx_request.write_into(&mut buffer);

let deserialized_tx_request = TransactionRequest::read_from_bytes(&buffer).unwrap();
assert_eq!(tx_request, deserialized_tx_request);
}
}
12 changes: 11 additions & 1 deletion tests/integration/custom_transactions_tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use miden_client::{
accounts::AccountTemplate, transactions::request::TransactionRequest, utils::Serializable, ZERO,
accounts::AccountTemplate,
transactions::request::TransactionRequest,
utils::{Deserializable, Serializable},
ZERO,
};
use miden_objects::{
accounts::{AccountId, AccountStorageType, AuthSecretKey},
Expand Down Expand Up @@ -149,6 +152,13 @@ async fn test_transaction_request() {
.unwrap()
.extend_advice_map(advice_map);

// TEST CUSTOM SCRIPT SERIALIZATION
let mut buffer = Vec::new();
transaction_request.write_into(&mut buffer);

let deserialized_transaction_request = TransactionRequest::read_from_bytes(&buffer).unwrap();
assert_eq!(transaction_request, deserialized_transaction_request);

execute_tx_and_sync(&mut client, transaction_request).await;

client.sync_state().await.unwrap();
Expand Down