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

Requesting names UI & Functionality #371

Merged
merged 1 commit into from
May 18, 2024

Conversation

robertmin1
Copy link

@robertmin1 robertmin1 commented Apr 23, 2024

Continuation of #363

@robertmin1 robertmin1 mentioned this pull request Apr 23, 2024
@robertmin1
Copy link
Author

One observation I can make is that we must transform the invoice into a paid status after sending it

@robertmin1
Copy link
Author

The cli currently lacks a "pay invoice" command. My plan is to focus on implementing for GUI exclusively, unless circumstances change.

@robertmin1
Copy link
Author

robertmin1 commented May 14, 2024

The other thing we should probably do is, make the "Check name availability..." button check whether a name_new is already in the wallet for that name (by doing the same check that the name_firstupdate command does), and if so, the UI should change to a new UI that only does name_firstupdate (as opposed to name_autoregister) -Jeremy

@JeremyRand
Copy link
Member

The cli currently lacks a "pay invoice" command. My plan is to focus on implementing for GUI exclusively, unless circumstances change.

I think the name_new RPC command will work fine for this, if you pass the destination, commitment, and amount params (can you confirm this?). It won't be able to accept these params in URI format, but that's an upstream limitation and isn't our problem.

@robertmin1
Copy link
Author

The other thing we should probably do is, make the "Check name availability..." button check whether a name_new is already in the wallet for that name (by doing the same check that the name_firstupdate command does), and if so, the UI should change to a new UI that only does name_firstupdate (as opposed to name_autoregister) -Jeremy

Is this the check I should be looking at

For the UI, I plan to open show_configure_name with a custom is_new parameter, such as firstupdate. Then using this parameter to modify the UI and its functionality here.

@JeremyRand

@robertmin1
Copy link
Author

robertmin1 commented May 14, 2024

The cli currently lacks a "pay invoice" command. My plan is to focus on implementing for GUI exclusively, unless circumstances change.

I think the name_new RPC command will work fine for this, if you pass the destination, commitment, and amount params (can you confirm this?). It won't be able to accept these params in URI format, but that's an upstream limitation and isn't our problem.

Yes, that should work, do we plug it into payto or create an independent command?

@JeremyRand
Copy link
Member

The cli currently lacks a "pay invoice" command. My plan is to focus on implementing for GUI exclusively, unless circumstances change.

I think the name_new RPC command will work fine for this, if you pass the destination, commitment, and amount params (can you confirm this?). It won't be able to accept these params in URI format, but that's an upstream limitation and isn't our problem.

Yes, that should work, do we plug it into payto or create an independent command?

Neither, AFAICT this should already work out of the box if you call name_new on the console with those params present. So there's nothing left to do CLI-wise unless I've missed something.

@JeremyRand
Copy link
Member

The other thing we should probably do is, make the "Check name availability..." button check whether a name_new is already in the wallet for that name (by doing the same check that the name_firstupdate command does), and if so, the UI should change to a new UI that only does name_firstupdate (as opposed to name_autoregister) -Jeremy

Is this the check I should be looking at

Yes. Feel free to factor that out into a separate function.

For the UI, I plan to open show_configure_name with a custom is_new parameter, such as firstupdate. Then using this parameter to modify the UI and its functionality here.

Yes that sounds fine; we already have an autoregister and update code path there, adding a 3rd firstupdate codepath should be straightforward.

@robertmin1
Copy link
Author

Alright!

@robertmin1
Copy link
Author

The other thing we should probably do is, make the "Check name availability..." button check whether a name_new is already in the wallet for that name (by doing the same check that the name_firstupdate command does), and if so, the UI should change to a new UI that only does name_firstupdate (as opposed to name_autoregister) -Jeremy

Is this the check I should be looking at

Yes. Feel free to factor that out into a separate function.

For the UI, I plan to open show_configure_name with a custom is_new parameter, such as firstupdate. Then using this parameter to modify the UI and its functionality here.

Yes that sounds fine; we already have an autoregister and update code path there, adding a 3rd firstupdate codepath should be straightforward.

If we make a separate the function, the password needs to be passed here This would mean prompting the user to enter the password for every check name availability?

@JeremyRand
Copy link
Member

JeremyRand commented May 16, 2024

If we make a separate the function, the password needs to be passed here This would mean prompting the user to enter the password for every check name availability?

Good point, I hadn't thought of that. OK, in that case, how about you instead edit the name_autoregister function, so that it attempts to call name_firstupdate by itself, and only does the name_new if name_firstupdate returns an error due to a missing input? This also improves UX for the CLI/RPC interface.

@robertmin1
Copy link
Author

This works well, definitely easier to implement

@JeremyRand
Copy link
Member

Code review passes for f784cae other than the things I noted above. Will test shortly.

@JeremyRand
Copy link
Member

Testing successful for f784cae other than the things I noted above.

@JeremyRand
Copy link
Member

ACK a24ce48

@JeremyRand JeremyRand merged commit 573b649 into namecoin:master-4.0.6 May 18, 2024
8 of 27 checks passed
@robertmin1 robertmin1 deleted the gifting-names branch May 19, 2024 09:29
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 this pull request may close these issues.

None yet

2 participants