Refactor asset handling and introduce SwapNoteStorage#2592
Refactor asset handling and introduce SwapNoteStorage#2592PhilippGackstatter merged 15 commits into0xMiden:nextfrom
Conversation
|
I wrote it similar to |
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good, thanks! I left a few comments.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
| storage.extend_from_slice(&requested_asset.as_elements()); | ||
| storage.extend_from_slice(payback_recipient.digest().as_elements()); | ||
| let inputs = NoteStorage::new(storage)?; | ||
| let inputs = NoteStorage::from(swap_storage); |
There was a problem hiding this comment.
nit: I'd rename the variable here from inputs to storage.
| let attachment_content = match attachment_kind { | ||
| NoteAttachmentKind::None => NoteAttachmentContent::None, | ||
| NoteAttachmentKind::Word => NoteAttachmentContent::new_word(attachment_content_word), | ||
| NoteAttachmentKind::Array => NoteAttachmentContent::new_word(attachment_content_word), | ||
| }; |
There was a problem hiding this comment.
This incorrectly maps NoteAttachmentKind::Array to NoteAttachmentContent::new_word but it should be new_array.
But the larger problem is that we cannot reconstruct the array content from the storage elements (&[Felt]), we'd need access to the advice map or pass the potential content of an array variant in some other way. So, in general, we cannot implement impl TryFrom<&[Felt]> for SwapNoteStorage.
For now, we do not need this impl, but it would be nice to have. There are some options:
- Remove the impl for now.
- Rethink whether a SWAP note needs to be as generic as it is: Does it need to be able to create payback P2ID notes with array attachments? Maybe it does, maybe not.
- Encode the array attachment in swap note storage so it is available for reconstruction. Makes it convenient here, but not such a good idea for on-chain usage.
- Implement
fn try_from_parts(storage: NoteStorage, payback_note_attachment: NoteAttachment)instead, where we would need to make sure the provided attachment matches the encoded kind and scheme instorageand then use the array content from the provided attachment.- After Allow multiple attachments per note and make them immutable #2555, when we have moved the attachment content out of the metadata, this would need to be slightly changed to take multiple attachments, but still overall possible.
Since we don't need it currently, I'd remove the TryFrom impl. If we do need this in the future, we can come back to this.
There was a problem hiding this comment.
yeah that was a bug. looked into it, and it makes sense to remove the TryFrom for now. i think once #2555 is merged, we can look back at this. Should I add a NOTE here?
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good, thanks!
3 participants
Closes #2515