Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove send_asset procedure from the basic wallet. #821

Closed
Fumuran opened this issue Aug 7, 2024 · 7 comments · Fixed by #829
Closed

Remove send_asset procedure from the basic wallet. #821

Fumuran opened this issue Aug 7, 2024 · 7 comments · Fixed by #829
Assignees

Comments

@Fumuran
Copy link
Contributor

Fumuran commented Aug 7, 2024

What should be done?

As soon as the #808 pull request will be merged, two new procedures cteate_note and move_asset_into_note will be available in the basic wallet. Used together they can substitute the existing send_asset procedure, so there is a proposal to remove send_asset from the basic wallet.

How should it be done?

send_asset procedure should be removed from the miden-lib/asm/miden/contracts/wallets/basic.masm and all its usages should be replaced with create_note and move_asset_into_note procedures.

When is this task done?

This task will be done when send_asset procedure will be entirely taken out of use and will be replaced with create_note + move_asset_into_note procedures.

Additional context

No response

@Fumuran Fumuran self-assigned this Aug 7, 2024
@bobbinth
Copy link
Contributor

bobbinth commented Aug 7, 2024

I'm curious what @Dominik1999 and @igamigo (and anyone else) think about this change. Basically, we'll be changing the basic wallet interface from 2 procedures: receive_asset and send_asset to 3 procedures: receive_asset, create_note, and move_asset_into_note.

@igamigo
Copy link
Collaborator

igamigo commented Aug 7, 2024

Other than the performance hit (which should be low/near neglibigle AFAICT) I don't see too much downside to the change. It seems nice to dynamically generate scripts as well.

@Dominik1999
Copy link
Collaborator

I don't see any downside to this approach. The only question is, how many people would send a note without an asset? We don't know that yet, because we don't have many users/builders. So, we should be open to changing that back once we realize that most users send assets via the wallet. In that case, we could have a third function, send_data_note, or something similar.

@Fumuran
Copy link
Contributor Author

Fumuran commented Aug 8, 2024

@Dominik1999 In that case send_data_note will work the same as send_asset now, right? Just sending one asset in one note? In that case we could probably leave the send_asset and see whether it will be used or not, and remove it if it won't.

how many people would send a note without an asset?

People will be able not only create a note without an asset, but also create a note and add several assets to it. But the same as for "note without assets", I don't know how often this feature will be used, but this is one of the advantages of using create_note + move_asset_into_note procs.

@Dominik1999
Copy link
Collaborator

That totally makes sense. Let's go for those three functions for now: receive_asset, create_note, move_asset_into_note

@bobbinth
Copy link
Contributor

bobbinth commented Aug 8, 2024

I guess we have a few options here:

1. Add new procedures to the existing wallet

Basically, basic wallet interface would have the following procedures

receive_asset
send_asset
create_note
move_asset_to_note

We already have this.

2. Replace send_asset with the new procedures

In this case, basic wallet interface would have the following procedures

receive_asset
create_note
move_asset_to_note

3. Move the new procedures into a separate interface

In this case, basic wallet will remain as:

receive_asset
send_asset

And we'd create a separate interface which would look like this:

create_note
move_asset_to_note

Not sure what we'd call this interface though - maybe "note sender" but I don't love it.

To my engineering brain, the 3rd approach seems a bit better. But it is also possible that I'm overcomplicating things here.

@Fumuran Fumuran linked a pull request Aug 13, 2024 that will close this issue
@bobbinth
Copy link
Contributor

Closed by #829.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants