feat: remove asset from create note #686
Conversation
Dominik1999
force-pushed
the
dominik_remove_ASSET_from_create_note
branch
from
df2e7de to
710ccaf
Compare
Dominik1999
changed the title
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline.
Probably the thing that took me the most time to review is handling of padding (and I'm not 100% sure I thought it through completely). For the next release, one of the first things we do should definitely be #685.
CHANGELOG.md
Outdated
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| ## 0.3.0 (TBD) | |||
|
|
|||
| * [BREAKING] Removed assets from `create_note` procedure and added new procedure `add_asset_to_note` (#686, #674). | |||
There was a problem hiding this comment.
minor nit: could we move this to the end of the list (I try to keep this list in ascending order).
There was a problem hiding this comment.
Jup,
I think the AccountStorageMap is missing in the list. Should I add it in this PR or should I open a new one only for it.
| # prepare the stack for return - stack has 5 elements too many | ||
| movupw.3 dropw swap drop |
There was a problem hiding this comment.
Why do we need to do this now while previously we didn't have to?
Separately, I wonder if a better approach here would be to call sys::truncate_stack. It would be a bit more expensive - but also generic in a way we won't need to worry about how many elements on the staack are extra. This could be done as a part of #685.
|
One other thing I think we should do is add a comment here to say that if there are no assets, |
Dominik1999
force-pushed
the
dominik_remove_ASSET_from_create_note
branch
from
06c72a3 to
82064b1
Compare
* feat: creating notes without assets * after review * pacifying linter * after review
* feat: creating notes without assets * after review * pacifying linter * after review
Closes #596
Supersedes #614
I remove
ASSETfrom thecreate_noteprocedure and the subsequent procedures. I also needed to adjust the event handler.