chore: upgrade protocol to RC3 - #2356
Conversation
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
Overall LGTM! Left some small comments I think we should address before merging.
| async fn chain_fee_faucet_id(client: &TestClient) -> Result<AccountId> { | ||
| let (genesis, _) = client | ||
| .get_block_header_by_num(BlockNumber::GENESIS) | ||
| .await? | ||
| .context("genesis block header is not in the store")?; | ||
| Ok(genesis.fee_parameters().fee_faucet_id()) | ||
| } |
There was a problem hiding this comment.
Let's inline this function call
|
|
||
| /// Builds a fee policy manager pricing every note the account can consume at zero. | ||
| async fn zero_fee_policy_manager( | ||
| client: &TestClient, |
There was a problem hiding this comment.
AFAICT, Client here is only used for getting the genesis, right? We can just pass the token faucet if that's the case
| NO_AUTH_PACKAGE, | ||
| MULTISIG_AUTH_PACKAGE, | ||
| ACL_AUTH_PACKAGE, | ||
| ]; |
There was a problem hiding this comment.
Can you check if there's new standards to add here?
| /// Builds a package and stores it under `{OUT_DIR}/{PACKAGE_DIR}` or | ||
| /// `{OUT_DIR}/{PACKAGE_DIR}/{subdirectory}` if a subdirectory is provided. | ||
| pub fn build_package( | ||
| package_name: &str, |
There was a problem hiding this comment.
Do we need package_name now that component_package is now of type Package?
| // state is changed. Provides no cryptographic authentication. | ||
| let no_auth_metadata = NoAuth::component_metadata(); | ||
| build_package("no-auth", NoAuth::code().as_library(), &no_auth_metadata, Some("auth")); | ||
| build_package("no-auth", NoAuth::code().as_package(), &no_auth_metadata, Some("auth")); |
There was a problem hiding this comment.
It's weird that we transform the code to package here and then re-create a new package within the build_package function
| assert!( | ||
| result.is_err(), | ||
| "an account with no fee asset balance should not be able to pay the fee" | ||
| ); |
There was a problem hiding this comment.
Let's match on a more specific error variant here
| &self, | ||
| code_interface: &AccountCodeInterface, | ||
| ) -> Result<Option<TransactionScript>, TransactionRequestError> { | ||
| ) -> Result<Option<(TransactionScript, Option<Word>)>, TransactionRequestError> { |
There was a problem hiding this comment.
Can you update the doc comments on this function and explain why this is now needed? Does the send tx script have non-zero args?
| /// Declares the asset the transaction fee is paid in, and the rate converting the chain's | ||
| /// native fee into it. | ||
| #[must_use] | ||
| pub fn fee_conversion_info(self, conversion_info: FeeConversionInfo, salt: Word) -> Self { | ||
| let (auth_arg, preimage) = commit_fee_conversion_info(conversion_info, salt); | ||
| self.auth_arg(auth_arg).extend_advice_map([(auth_arg, preimage)]) | ||
| } |
There was a problem hiding this comment.
Let's specify in the doc comments that only accounts with fee-conversion-aware components will work. Is there any way we can validate this before executing and showing a nice error if the account does not support it?
| /// Aggregates individual attachment commitments into the note's attachments commitment. | ||
| /// | ||
| /// The element layout mirrors [`NoteAttachments`]' own sequential commitment, so hashing this | ||
| /// yields the same value as the full attachments would, without needing their contents. | ||
| struct AttachmentCommitments(Vec<Word>); | ||
|
|
||
| impl SequentialCommit for AttachmentCommitments { |
There was a problem hiding this comment.
Let's add a TODO here referencing issue #2360 for making better use of this new change
| impl TryFrom<proto::note::NoteSyncMetadata> for NoteMetadata { | ||
| type Error = RpcConversionError; |
There was a problem hiding this comment.
Let's also add a similar TODO here
3 participants
Bump miden dependencies to their latest git
nextversions.Depends on 0xMiden/node#2421 getting merged into its own
next.