feat: add serialization for TransactionRequest
#471
Conversation
tomyrd
force-pushed
the
tomyrd-tx-request-serialization
branch
from
7a14d70 to
c587460
Compare
| 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 |
There was a problem hiding this comment.
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.
| } | ||
| 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); |
There was a problem hiding this comment.
Is there a way to avoid cloning the advice map here?
There was a problem hiding this comment.
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
AdviceMapso we can usewrite_into. - Implement
.iter()to iterate over a reference.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added 0xPolygonMiden/miden-vm#1448 for this
closes #466
This PR implements serialization to and from bytes for
TransactionRequest. Some tests are also added to verify a correct serialization.When comparing if the deserialized
TransactionRequestis the same, we are ommitting theProgramAstcode as we found that they were not the same. The difference was mentioned by a comment inmiden-vm, which was later removed in v0.10. Once the client is updated to the new vm version we can add back the check to see if the problem persists.