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

[4][ahdigital] UX Account page, Proposed transactions #1284

Closed
abitmore opened this issue Mar 9, 2018 · 39 comments
Closed

[4][ahdigital] UX Account page, Proposed transactions #1284

abitmore opened this issue Mar 9, 2018 · 39 comments
Labels
[6] UX Impact flag identifying the application User Experience
Milestone

Comments

@abitmore
Copy link
Member

abitmore commented Mar 9, 2018

Two issues:

  1. Missing translation.
  2. If there are several operations in one proposal, only need to show proposal ID and expiration once.

image

An old light wallet:
image

@clockworkgr
Copy link
Member

May I also suggest a more obvious separator between proposals.

At the example above it's almost impossible to see where one set of Ops ends and the next proposal's set starts.

@startailcoon
Copy link
Contributor

I can work on this

@startailcoon startailcoon self-assigned this Mar 11, 2018
@wmbutler wmbutler added [3] Feature Classification indicating the addition of novel functionality to the design [6] UX Impact flag identifying the application User Experience labels Mar 12, 2018
@wmbutler wmbutler changed the title Account page, Proposed transactions [2][2] Account page, Proposed transactions Mar 12, 2018
@wmbutler
Copy link
Contributor

2 for UX and 2 for execution. @ahdigital ?

@wmbutler wmbutler added this to the 180401 milestone Mar 12, 2018
@ahdigital
Copy link
Contributor

No problem.

@ahdigital
Copy link
Contributor

ahdigital commented Mar 15, 2018

You'll have to bear with me as I'm still understanding how the proposed transactions functionality works, but I think this would address the issues raised here.

image

Updates include:

  • Displaying ID and expiration date once per proposal
  • Added a column showing who the approver is
  • Collapsed the approvers in the status column
  • Aligned cells to top
  • Added a border to separate proposals

Question

I added 'pending' into the status column but I wasn't sure whether I should color code these like the example below:

image

E.g. when percentage >100 show green. Would this be useful? Obviously the 2nd proposal would never go green because as soon as it is approved it is removed from the table.

@abitmore
Copy link
Member Author

@ahdigital proposals can be in pending status due to several reasons:

  • not approved, or
  • review time is non-zero, or
  • failed to execute on approval due to insufficient balance or something else.

So it's still possible that the "pending" status of the 2nd proposal will go green if green only means >=100%.

The "approver" column is not clear enough to me. A transaction can have several operations, which means "approver" of each operation can be different, so we can have multiple account names for each proposal, E.G. the scenarios described in #866 and #1132.

@ahdigital
Copy link
Contributor

Thanks for feedback.

Have I got the first row of the table correct? E.g showing one proposal but with two operations. Is that what you meant in your original post?

@wmbutler wmbutler changed the title [2][2] Account page, Proposed transactions [4][ahdigital][2][startailcoon] Account page, Proposed transactions Mar 15, 2018
@wmbutler wmbutler changed the title [4][ahdigital][2][startailcoon] Account page, Proposed transactions [4][ahdigital][3][startailcoon] Account page, Proposed transactions Mar 15, 2018
@abitmore
Copy link
Member Author

Have I got the first row of the table correct? E.g showing one proposal but with two operations. Is that what you meant in your original post?

Yes.

@abitmore
Copy link
Member Author

By the way, due to the vary reasons that a proposal got "approved" but didn't execute, it's best if the UI can indicate the reason. However, in case when it failed to execute, current back end doesn't return the reason (would be better when bitshares/bitshares-core#729 and/or bitshares/bitshares-core#730 is implemented).

@ahdigital
Copy link
Contributor

The "approver" column is not clear enough to me. A transaction can have several operations, which means "approver" of each operation can be different

Does this mean there would be a status for each operation as well? Or is status always the 'overall' status of a proposal?

@ahdigital
Copy link
Contributor

ahdigital commented Mar 16, 2018

Whilst I wait for your response, here is the latest revision, which includes:

  • Multiple approvers for when a proposal has multiple operations
  • Colour coded statuses with tooltip explanations*
  • Failed proposals with a remove button so that they can be removed from the list

*Until BS-core supports failed execution messages it's probably best to leave the statuses in white.

Orange colored 'pending' could mean not approved, or review time is non-zero and red colored 'failed' could mean whatever error message returned from BS-core. With this approach there'd be no status in green on the basis that anything approved without failure gets removed from the list anyway.

image

@abitmore
Copy link
Member Author

Does this mean there would be a status for each operation as well? Or is status always the 'overall' status of a proposal?

Only an overall status.

Failed proposals with a remove button so that they can be removed from the list

  • Failed proposals will be retried when it expires;
  • after the first fail, for example due to insufficient balance, conditions can change, for example someone can transfer some funds to the account;
  • anyone can trigger a retry by adding an approval to the proposal or removing an approval from the proposal;
  • although "remove" is possible now which is another on-chain operation, currently the underlying design is flawed and the back end code is buggy, so I would recommend not show it so far
    • the proposals that didn't execute can be removed as well

@ahdigital
Copy link
Contributor

ahdigital commented Mar 21, 2018

Thanks for the tips. In this revision, I've removed the 'remove' button and added 2 additional dummy proposals to represent the scenarios described.

proposed transactions v3

1.10.8740
Shows a proposal that failed, and where the user previously neither approved or rejected but can trigger a retry by clicking either option.

1.10.8741
Shows a proposal that failed, where the user previously approved but can trigger a retry by clicking either option.

1.10.8742
Shows a proposal that failed, where the user previously rejected but can trigger a retry by clicking either option.

@abitmore
Copy link
Member Author

Thanks for your efforts.

The action "approve" means adding an approval (turn on the "v" icon) to the proposal, "reject" means removing an approval (turn off the "v" icon). So,

  • if the proposal is not approved by anybody, people can only add approval, or say, can only approve;
  • if the proposal has been approved by somebody, people can reject or approve.

However, it's more tricky than that:

  • an approval from an unrelated account can be added to any proposal, which can trigger a retry!! It can also be removed after added. For better UX, perhaps we won't allow this in UI.
  • "approve" and "reject" are actions, which means theoretically anyone can propose another proposal to "approve" or "reject" an existing proposal!! (although it's not possible now due to a bug in back end: Unable to propose a proposal with an approve_proposal operation bitshares-core#214)
  • theoretically owner permission can be used to do anything that requires active permission, however there is another bug in back end which causes it's only able to do most of actions but not all, which is hard to fix and makes the situation more complicated: Owner keys of non-immediately required accounts can not authorize a transaction bitshares-core#584. In this example, I assume the bug doesn't exist: if there is a proposal contains an operation that transfer 1 BTS from A to B, A's owner permission is (threshold=2, account-c, weight 1, account-d, weight 1), A's active permission is (threshold=2, account-e, weight 1, account-f, weight 1), then we can approve the proposal by these combinations:
    • c's active + d's active
    • c's owner + d's active
    • c's active + d's owner
    • c's owner + d's owner
    • e's active + f's active
    • e's owner + f's active
    • e's active + f's owner
    • e's owner + f's owner

I'd admit the UX design is very hard.

@wmbutler
Copy link
Contributor

@ahdigital @startailcoon just keep adding hours as needed. We support this effort.

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Mar 26, 2018

If bitshares/bitshares-core#732 gets accepted, the UI could also provide a history of proposals (if the backend node has the plugin active).

@ahdigital
Copy link
Contributor

@abitmore - Are the combinations in your example correct?

A's owner permission is (threshold=2, account-c, weight 1, account-d, weight 1)

account-e and account-f aren't listed in owners authority list.

A's active permission is (threshold=2, account-e, weight 1, account-f, weight 1)

account-c and account-d aren't listed in active authority list.


I've been talking this through with @sschiessl-bcp and it seems that a proposal cannot be approved by combining authorities from different permission groups. E.g. Active and owner permission groups are separate definitions that cannot be combined to approve a single proposal.

The combinations for approval for each group are calculated separately like this I believe:

Active permissions group

  • c's active + d's active
  • e's active + f's active

Owner permissions group

  • e's owner + f's owner
  • c's owner + d's owner

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Mar 26, 2018

I might have also misunderstood it, I am not too familiar with the actual backend logic there, @abitmore is much more qualified.

Anyways, assuming that you have
A's owner permission is (threshold=2, account-c, weight 1, account-d, weight 1)
A's active permission is (threshold=2, account-e, weight 1, account-f, weight 1)

then I thought the following combinations give

  • c's active + d's active: not approved, owner permission required
  • c's owner + d's active: not approved, owner permission required
  • c's active + d's owner: not approved, owner permission required
  • c's owner + d's owner: approved, both owner present
  • e's active + f's active: approved, both active present
  • e's owner + f's active, e's active + f's owner: Owner permission also gives active authority, the question is: Does the calculation that sums up the threshold allow mix of present permissions?
  • e's owner + f's owner: approved, owner permission grants active authority as well

@abitmore
Copy link
Member Author

c's active + d's active: not approved, owner permission required

No, owner permission of c or d is not required, it's owner permission of a that is qualified for approve the transaction, by setting c and d as a's owner cosigner, c and d can sign with their own owner or active keys to do things that requires a's owner or active permission.

@abitmore
Copy link
Member Author

Easy to test in testnet. There do have some scenarios affected by bitshares/bitshares-core#584.

@sschiessl-bcp
Copy link
Contributor

Oh I see. I thought that granting owner permission also requires owner permission of the cosigner accounts, but was just my hunch and not backed by actual backend knowledge. Thanks for clarification @abitmore

@ahdigital
Copy link
Contributor

So, what's the final correct set of combinations for the example above?

@ahdigital
Copy link
Contributor

ahdigital commented Mar 27, 2018

I've been using testnet to try out different scenarios, a couple are documented below:

Scenario A

I've unknowingly created a situation where I keep getting active authority errors.

Permissions for account: test-010101 (cloud wallet)

image

Operation scenarios

  1. Transfer 1000 TEST from test-010101 to test-020202
  2. Remove test-020202 from active permission list

Operation results

  1. missing required active authority: Missing Active Authority 1.2.20940
  2. missing required active authority: Missing Active Authority 1.2.20940

Scenario B

Permissions for account: test-040404 (cloud wallet)

image

Operation scenarios

  1. Transfer 1000 TEST from test-040404 to test-020202
  2. Remove test-020202 from active permission list
  3. Add test-020202 to active permission list

Operation results

  1. Transaction confirmed
  2. Transaction confirmed
  3. Transaction confirmed

Observations / questions

I keep reading this general statement: "The owner permission can do everything". In any of my tests, when I proposeed a transaction I didn't see anywhere where an owner can approve a transaction. All I saw was active authorities listed on the proposed transactions page. Further to this:

an approval from an unrelated account can be added to any proposal

I couldn't approve proposals from unrelated accounts, and instead got missing required active authority error.

Something else I'm wondering is that if I have authorities in my active permissions list, why is the 'propose' option on modals optional? What is the point in defining permissions for transactions, that I can then voluntarily circumvent?

Design revision

Following the feedback from @abitmore and now bearing in mind that 'reject' only means that you remove your own approval I've updated the buttons on the design from earlier.

proposed transactions v4

I feel this design is an improvement on what we have right now, but the scope is increasing as more caveats are raised. I have no problem continuing to accommodate these but it's difficult when the overarching functionality isn't widely understood or documented or is buggy. Not knocking anyone - support from @abitmore and @sschiessl-bcp has been great.

@sschiessl-bcp
Copy link
Contributor

Thank you for the test details!

Scenario A
Who signed case 1) and 2)? I suspect that you tried to execute the operation directly instead of proposing it?

Scenario B
Who signed cases 1) 2) and 3)?

@ahdigital
Copy link
Contributor

In all scenarios where I was creating an operation I was using the account that setup the permissions lists. So, scenario A was test-010101 and B was test-040404.

In scenario A, this is what I was doing to get the active authority error:

scenario-a

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Mar 27, 2018

In this case you would need to propose the proposal due to the setup. To create the proposal you need active authority, but you can't get it because its a multisig account.

What you need to do is: Create the proposal from a third account (e.g. the test-020202 account) that only has one active key. I just created one for test-010101 from one of my test accounts (the Send (legacy) page supports that).

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Mar 27, 2018

Something else I'm wondering is that if I have authorities in my active permissions list, why is the 'propose' option on modals optional? What is the point in defining permissions for transactions, that I can then voluntarily circumvent?

You can only try to create the operation, but your example above shows that it won't work. In your example you did try to create an operation from a multisig account, and you failed because you could only use your own authority. The UI could be more clear here, yes. But I think it doesnt need to prevent it, rather have an understandable error message at the end.

@clockworkgr
Copy link
Member

My understanding is that the statement "The owner permission can do everything" refers simply to the achieveable results.

In the sense that the owner can remove any and all active authorities set a new one and perform whatever transaction etc thus being in complete control of the account. Don't think it was supposed to act as an 'override' in terms of allowed operations.

@sschiessl-bcp
Copy link
Contributor

My understanding is that the statement "The owner permission can do everything" refers simply to the achieveable results.

Yes, its two levels: what an obtained permission can do, and how to obtain said permission. Your problems evolve around obtaining the permission in a multi-sig account.

@abitmore
Copy link
Member Author

I keep reading this general statement: "The owner permission can do everything". In any of my tests, when I proposeed a transaction I didn't see anywhere where an owner can approve a transaction. All I saw was active authorities listed on the proposed transactions page.

Not all possible options are showing on current UI, that's why we're discussing in this ticket.
E.G. if a transaction requires active permission, current UI only shows accounts in active permission set.

an approval from an unrelated account can be added to any proposal
I couldn't approve proposals from unrelated accounts, and instead got missing required active authority error.

Because current UI doesn't show unrelated accounts.

Something else I'm wondering is that if I have authorities in my active permissions list, why is the 'propose' option on modals optional? What is the point in defining permissions for transactions, that I can then voluntarily circumvent?

To schedule the transaction to execute at a later time; or simply create a proposal as a reminder.

that 'reject' only means that you remove your own approval I've updated the buttons on the design from earlier.
image

Sometimes need to show both buttons If have more than one account in the wallet.

Actually, according to #1285, don't hide or disable action buttons due to insufficient permission. That said, please ignore the bitshares/bitshares-core#214 bug, always show "approve" button; if a proposal is approved by someone already, show "reject" button.

@startailcoon startailcoon removed their assignment Mar 27, 2018
@startailcoon
Copy link
Contributor

I have to release this task until the details of it has been settled and defined. It's free for claims for anyone else if they wish. I have enough this milestone as it is atm.

@startailcoon startailcoon changed the title [4][ahdigital][3][startailcoon] Account page, Proposed transactions [4][ahdigital][3] Account page, Proposed transactions Mar 27, 2018
@wmbutler
Copy link
Contributor

@ahdigital keep at it. Will continue to support the hours dedicated to it.

@calvinfroedge
Copy link
Contributor

I might suggest a new issue with a focused scope when design is all sorted.

@sschiessl-bcp
Copy link
Contributor

I think it's fine for the logic now, much clearer than before. We don't need to reinvent it.

Like abit mentioned "always show "approve" button; if a proposal is approved by someone already, show "reject" button.".
For the status logic the multisig setting shouldn't matter too much.

@wmbutler
Copy link
Contributor

wmbutler commented Apr 1, 2018

Who will claim the work portion of this? Can you wrap up UX with what has been discussed here? @ahdigital

@wmbutler wmbutler modified the milestones: 180401, 180415 Apr 1, 2018
@ahdigital
Copy link
Contributor

Summary

The main updates to this page are (screenshot reference at the end):

  • A proposal can be made up of one or more operatons (E.g. 1.10.8626)
  • We now display ID and expiration date once per proposal
  • There is an approvers column to show who the approver of each operation is
  • The list of multi sigs required in the status column are expandable/collapsable
  • The status in the status column will either be 'Pending' or 'Failed' with the tooltip to show message if/when one is supplied (E.g. 1.10.8740)
  • Proposals are now have a visual separation (bottom border)
  • Table cells should all be aligned to top
  • The approve button should always be visible
  • The reject button only appears once the user has clicked approve on a proposal (E.g. 1.10.8739)

To whoever takes on the development portion of this, there was a lot of discussion in this thread about existing functionality and inner workings of proposals in general. However, I don't think there's any fundamental changes required right now. I agree with @calvinfroedge, let's polish the existing experience and create new issues for bug fixes or implementing updates when made available via Bitshares Core.

image

@wmbutler
Copy link
Contributor

I expect this will require > 3 hours of dev work as well.

@wmbutler wmbutler modified the milestones: 180418, 180501 Apr 21, 2018
@ahdigital
Copy link
Contributor

Bump - can anyone take on the development?

@wmbutler wmbutler removed the [3] Feature Classification indicating the addition of novel functionality to the design label May 3, 2018
@wmbutler wmbutler changed the title [4][ahdigital][3] Account page, Proposed transactions [4][ahdigital] UX Account page, Proposed transactions May 3, 2018
@sschiessl-bcp
Copy link
Contributor

Did you do anything in that matter already @startailcoon ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[6] UX Impact flag identifying the application User Experience
Projects
None yet
Development

No branches or pull requests

7 participants