Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Allow deriving new addresses #71

Closed
8 tasks done
mvaivre opened this issue Dec 8, 2021 · 6 comments
Closed
8 tasks done

Allow deriving new addresses #71

mvaivre opened this issue Dec 8, 2021 · 6 comments
Assignees
Labels
epic Big task, composed of multiple subtasks

Comments

@mvaivre
Copy link
Member

mvaivre commented Dec 8, 2021

Users need to be able to derive multiple addresses, for multiple groups.
This will be especially useful when interacting with dApps.

Here are the 3 scenarios we foresee for address derivation:

  • Derive address in random group (by default)
  • Derive address in defined group
  • Derive one address per group (mining purposes)
    Derive address for defined path

Design

  • Create mockups (goal is to keep friction low, by hiding advanced features)

Other tasks

@mvaivre mvaivre added the epic Big task, composed of multiple subtasks label Dec 8, 2021
@mvaivre mvaivre added this to the v1.2.0 milestone Dec 13, 2021
@mvaivre
Copy link
Member Author

mvaivre commented Jan 10, 2022

Mockups can be seen here. Feedback are welcome.

We'll start with deriving addresses for a random group or a defined group.

Deriving an address for a defined path is postponed, as it could make address discovery more difficult.
We should take some time to asses its usefulness (for now, advanced users can use the node wallet) and think about how users could benefit from such a functionality (UX perspective).

@nop33
Copy link
Member

nop33 commented Jan 17, 2022

As agreed in a call with @mvaivre and @tdroxler, and as it can be seen in the mockups:

  • In the addresses page, the user will be able to click "Derive new address"
  • This opens up a modal where the user can optionally specify the group they would like the address in
  • If no group is given, an address win a random group will be generated by calling the appropriate method locally.
  • This method accepts the path as an argument. We increment the last segment of the path until we have the address in the group that we want.
  • To save computation time every time the user logs in into their wallet, we store an array of indexes (the last segment of the path) of the addresses that were generated by the user, and we only generate those.

@nop33 nop33 self-assigned this Jan 18, 2022
nop33 added a commit that referenced this issue Jan 25, 2022
nop33 added a commit that referenced this issue Jan 25, 2022
nop33 added a commit that referenced this issue Jan 25, 2022
@nop33
Copy link
Member

nop33 commented Jan 31, 2022

@polarker, let's move the discussion here. This is where @mvaivre posted the mockups, and your question in my PR is related to the designs, not the PR itself: #120 (comment)

The balance shown on the top left is the total amount of all addresses right? This might cause trouble for transactions, as users might think that the balance is enough on his main address.

Maybe we should do like Metamask and only show on the overview page with the balance of the main address. Another advantage of Metamask’s approach is that users always know that they are using one of the addresses when doing all of the operations.

Or another approach is to show the main address and the balance of the address on top left. I am just thinking out loud

@mvaivre
Copy link
Member Author

mvaivre commented Jan 31, 2022

Thanks @nop33 for centralising the discussion here, where it should be.

@polarker An important friendly note about working habits : the mockups have been shared here 3 weeks ago in order to be reviewed and to collect feedback. I've also pasted the link here and there (Github, Slack, PM) to make sure those are actually consulted.

Talking about the design and flows after implementation is really something we should avoid. Detailed mockups are here to avoid exactly that. Thanks in advance for your understanding!

Regarding your comment, have you checked the updated "Send" modal in the mockups? The amounts available in each address is clearly indicated in order to avoid confusion.
Also, the "main address" concept is here to ensure that this address is always used by default for all operations.

@polarker
Copy link
Member

polarker commented Jan 31, 2022

@mvaivre I think I try to spot the issues as early as possible, but it's not always possible. At least, it's never possible for backend design. We have to investigate (where we brainstorm), prototype implementation (where we might spot more issues), and go for final design. Even after the final design, we would still refactor the code if it's needed.

I only had the feedback until I saw @nop33's video. Sorry that I could not come up with such feedbacks when seeing the mockups, but apparently video provides more information than mockup.

I guess we should always commit to truth not to consistency. Let's have a call to discuss the design now that we have more ideas. Of course, if my feedback makes sense, we could refactor the design at a later stage.

Edit: even if we need to refactor, most of the things can still be reused as far as I see, and it's better to refactor as early as possible if it's necessary to refactor for the long term.

@nop33
Copy link
Member

nop33 commented Mar 15, 2022

I believe we can close this issue since the features have been implemented! We are in the testing phase before releasing. Any futher bug fixes can be reported individually and added in the v1.2.0 milestone.

@mvaivre mvaivre closed this as completed Mar 16, 2022
LeeAlephium pushed a commit that referenced this issue May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
epic Big task, composed of multiple subtasks
Projects
None yet
Development

No branches or pull requests

3 participants