feat: TransactionRequest FPI support
#560
Conversation
igamigo
force-pushed
the
igamigo-fpi
branch
3 times, most recently
from
c58ce85
to
110ec0d
Compare
|
The test is blocked by 0xPolygonMiden/miden-base#934, but otherwise this PR can be reviewed |
|
I've merged 0xPolygonMiden/miden-base#936. We should update this PR to use this new method. |
There was a problem hiding this comment.
Thank you! Looks good! Not a full review, but I left some comments inline.
I think the biggest thing is to make sure we load account code into the executor based on the changes in 0xPolygonMiden/miden-base#936.
|
|
||
| for account_proof in account_proofs.into_iter() { | ||
| let account_header = account_proof.account_header().ok_or_else(|| { | ||
| ClientError::RpcError(RpcError::ExpectedDataMissing("AccountHeader".to_string())) |
There was a problem hiding this comment.
nit: it feels a bit weird to return RPC errors from here - but I guess we do this in quite a few places.
Not for this PR, but I'd probably have a specialized endpoint in NodeRpcClient - e.g., something like get_foreign_account_data() which would internally call get_account_proofs() and would map the response to something like ForeignAccountData structs which don't require this type of validation here.
There was a problem hiding this comment.
Agreed, I'll create an issue before merging.
| * Allow to set expiration delta for `TransactionRequest` (#553). | ||
| * Implemented `GetAccountProof` endpoint (#556) | ||
| * Implemented `GetAccountProof` endpoint (#556). |
There was a problem hiding this comment.
I think this change accidentally got duplicated in the merge
| tx_args.extend_merkle_store(store.inner_nodes()); | ||
| foreign_account_codes | ||
| .iter() | ||
| .for_each(|(_, code)| self.tx_executor.load_account_code(code)); |
There was a problem hiding this comment.
We are permanently mutating the tx_executor here. Wouldn't it be a problem for the executor to have old account code in subsequent executions?
| // Inject foreign account data | ||
| let (foreign_data_advice_inputs, foreign_account_codes) = self | ||
| .get_foreign_account_inputs(transaction_request.foreign_account_data()) | ||
| .await?; | ||
| let mut tx_args = transaction_request.into_transaction_args(tx_script); | ||
| let AdviceInputs { map, store, .. } = foreign_data_advice_inputs; | ||
| tx_args.extend_advice_map(map); | ||
| tx_args.extend_merkle_store(store.inner_nodes()); |
There was a problem hiding this comment.
I feel like this part should be in TransactionRequest::into_transaction_args but I just realized get_foreign_account_inputs does an rpc call so it would be strange. Maybe we could receive the foreign data advice inputs in with_foreign_public_accounts instead of retrieving it inside new_transaction.
We could even store and expose the account codes so that we don't have to repeat the rpc call when loading the code to the executor.
There was a problem hiding this comment.
The RPC call is for more than the account code so we probably cannot avoid it. Also, the endpoint has a mechanism for passing last-known account codes for the requested Account IDs (I added a TODO for this, will create an issue before merging).
Retrieving the foreign account data outside of new_transaction is not really an option because it's relevant only at execution time (ie, if I am some service that serializes a TransactionRequest so that you execute it, it makes sense that you retrieve fresh data when you execute)
| let tx = client | ||
| .new_transaction( | ||
| foreign_account_id, | ||
| TransactionRequest::new().with_custom_script(deployment_tx_script).unwrap(), |
There was a problem hiding this comment.
I think the with_custom_script isn't needed here. If no script or own notes are specified to the TransactionRequest then the script will only have the auth call, like the custom script we created here.
There was a problem hiding this comment.
Ah, good catch, thanks
There was a problem hiding this comment.
Actually, while it does default to the script, it is built with the assumption that you are consuming notes, which is not the case here so it would error.
Closes #530