fix(standards): derive the network account target of MINT and BURN notes - #3664
fix(standards): derive the network account target of MINT and BURN notes#3664onurinanc wants to merge 11 commits into
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! Not a full review, but I left a couple of questions inline.
| return Self::ensure_presence(attachments, target_id); | ||
| } | ||
|
|
||
| Self::contains_target(attachments, target_id).map(|_| ()) |
There was a problem hiding this comment.
We need this line to check if a private account has malformed network target attachment, right? Or is there some other reason?
Regardless - we should add a brief comment explaining why this line is needed.
| NetworkAccountTarget::ensure_presence_if_public(&mut attachments, storage.faucet_id()) | ||
| .map_err(|err| { | ||
| NoteError::other_with_source("failed to target the MINT note at its faucet", err) | ||
| })?; |
There was a problem hiding this comment.
Why for MINT notes we use ensure_presence_if_public() but for BURN notes we use just ensure_presence()? Couldn't both of them be directed to non-network accounts?
| /// The attachment is the canonical target encoding the network routes on; the consume-side bind is | ||
| /// the asset itself, which the faucet's `receive_and_burn` rejects if it did not issue it. |
There was a problem hiding this comment.
| /// The attachment is the canonical target encoding the network routes on; the consume-side bind is | |
| /// the asset itself, which the faucet's `receive_and_burn` rejects if it did not issue it. |
we can easily skip the extra detail without losing the important information
| /// - the attachments exceed their protocol limit (see [`NoteAttachments::new`]); the target | ||
| /// attachment occupies one of the available slots when the caller does not supply it. |
There was a problem hiding this comment.
| /// - the attachments exceed their protocol limit (see [`NoteAttachments::new`]); the target | |
| /// attachment occupies one of the available slots when the caller does not supply it. | |
| /// - the attachments exceed their protocol limit (see [`NoteAttachments::new`]). |
| /// its storage, which is also what the note is tagged for. The attachment is the canonical target | ||
| /// encoding the network routes on; the consume-side bind is the stored `ASSET_ID`, which the |
There was a problem hiding this comment.
ditto; should be shortened
| /// [`NoteAttachments::new`]). | ||
| /// Returns an error if: | ||
| /// - the attachments carry a `NetworkAccountTarget` for an account other than the faucet. | ||
| /// - the attachments exceed their protocol limit (see [`NoteAttachments::new`]); the target |
|
|
||
| /// The builder attaches the network target for the minting faucet, so the note is a network | ||
| /// note without the caller having to add the attachment. | ||
| #[test] | ||
| fn builder_attaches_network_target() { | ||
| let mint_note = build_mint_note(faucet(), Vec::new()).unwrap(); | ||
|
|
||
| assert_eq!(mint_note.attachments().num_attachments(), 1); | ||
|
|
||
| let network_note = AccountTargetNetworkNote::new(Note::from(mint_note)).unwrap(); | ||
| assert_eq!(network_note.target_account_id(), faucet()); | ||
| assert_eq!(network_note.execution_hint(), NoteExecutionHint::Always); | ||
| assert!(network_note.as_note().is_network_note()); | ||
| } |
There was a problem hiding this comment.
AFAICS, the set of new tests added is the same for mint and burn - I'd aim to de-deduplicate code if possible
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo verbose/unclear comments
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
LGTM, aside from the one comment.
| // No target is derived, but a caller-supplied attachment of the scheme must still be | ||
| // validated, so the note cannot claim a network target it does not have. The returned flag | ||
| // is discarded because it is always false here: an attachment naming a non-public account | ||
| // never decodes into a `NetworkAccountTarget`, so a present target can only be an error. | ||
| Self::contains_target(attachments, target_id).map(|_| ()) |
There was a problem hiding this comment.
I'm struggling to understand the comment, it's very claude-ish. I think the idea is to make sure there is no attachment that a user added that claims to be a NetworkAccountTarget but actually contains a private target_id. If so, I would suggest something like:
// Ensure that none of the user-provided attachments claims to be a
// `NetworkAccountTarget` with a private target.
| // Reject any attachment the caller added under the target scheme: naming another account | ||
| // is a mismatch, and naming this private one does not decode as a `NetworkAccountTarget`. |
There was a problem hiding this comment.
TBH I still struggle to understand this comment.
I think what makes it worse is the name contains_target.
At first, I'd expect that a NetworkAccountTarget encoding a private account still "contains" this private account as target (even though the constructor won't allow such case).
A better fit might be validate_target: then describing that a network attachment with private account fails to "validate" reads much more intuitively.
So maybe we could rename contains_target -> validate_target?
4 participants
Closes: #3663