Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Set transaction parameters manually (advanced) #838

Closed
lukasschor opened this issue Apr 29, 2020 · 56 comments · Fixed by #1760
Closed

Set transaction parameters manually (advanced) #838

lukasschor opened this issue Apr 29, 2020 · 56 comments · Fixed by #1760
Assignees
Labels
Feature 👑 New functionality

Comments

@lukasschor
Copy link
Member

lukasschor commented Apr 29, 2020

What is this feature about? (1 sentence)

When initiating a new transaction, it is possible to set the transaction nonce, gas price and gas limit manually as an advance feature.

Why is it needed? What is the value? For whom do we build it?

  • As an advanced user I want to be able to set the transaction parameters manually so that I can have full control over the transaction for special use cases or in case there are some issues with the interface
  • Some owner wallets such as hardware wallets don't allow to modify the gas limit and price later on. We got feedback several times that users are missing this.

High-level overview of the feature

  • This feature should work for all tx modals, i.e. send assets, contract interaction, settings change, Safe apps, etc.

  • This is an advanced feature, meaning it should not clutter the interface for regular users.

    • There could be an advanced section on the tx modal access this hidden feature (e.g. through a gear icon or a "advanced settings" dropdown or similar)
    • Alternative: First activate this in the Safe settings (we could introduce a new "Advanced features" tab in the settings where features such as this can be toggled on/off).
  • Parameters to set in this version:

    • Safe transaction parameters: Safe nonce, safeTxGas
    • Ethereum transaction parameters: Nonce, Gas limit, Gas price
  • Note: Safe transaction parameters will be taken into account no matter if the confirmation happens on or off-chain. Ethereum tx params are only relevant for on-chain confirmations and executions. But we don't know that upfront.

  • Validation:

    • Do not accept negative values when editing parameters
    • SafeTxGas can not be lower than Ethereum gas limit
  • Modals that need modifications:

    • Send funds review
    • Contract interaction review (with ABI)
    • Contract interaction review (without ABI, using encoded data)
    • Send collectible review
    • Safe Apps
    • Tx Table (reject)
    • Advanced options
      • owner (add - remove - replace)
      • Modify policies
      • Spending limit (add - remove)

Updates

  • Nov 5, Tobi: Add more details
  • Nov 5, Tobi: Added params and note
  • Jan 6, Nico: Validations and Modals
@lukasschor lukasschor added Feature 👑 New functionality Major Needs to be fixed for immediate next public release. labels Apr 29, 2020
@tschubotz
Copy link
Member

Also to consider: When using our direct Ledger integration, users aren't able to set the gas price on the Ledger. So that would have to be done via our interface (cf. gnosis/safe#314)

@lukasschor lukasschor changed the title Set gas price in the interface Set transaction parameters manually May 3, 2020
@lukasschor lukasschor changed the title Set transaction parameters manually [Advanced] Set transaction parameters manually May 18, 2020
@tschubotz
Copy link
Member

Just received some user feedback where the following scenario leads to inconveniences:

- 2/3 Safe with owners a,b,c
- there's a disagreement, a and b want to remove c
- before they can trigger the tx, owner c "spams" the safe with transactions so send all company funds to itself, re-creating the same txs multiple times.
-> Currently, A and B would have to manually reject all those spam txs before they can get their tx to remove the owner through.

@tschubotz tschubotz removed the Major Needs to be fixed for immediate next public release. label Oct 6, 2020
@tschubotz tschubotz changed the title [Advanced] Set transaction parameters manually Set transaction parameters manually Nov 5, 2020
@tschubotz
Copy link
Member

@rmeissner

I have 2 open questions about this feature:

  • Should we differentiate between base gas and safetxgas?
  • Should this feature only be available when the user executes a tx or confirms on-chain, i.e. not for off chain signatures?

I think "yes" in both cases. Do you agree?

@tschubotz
Copy link
Member

@posthnikova this is another upcoming web feature that would require design.

@lukasschor
Copy link
Member Author

@rmeissner

I have 2 open questions about this feature:

  • Should we differentiate between base gas and safetxgas?
  • Should this feature only be available when the user executes a tx or confirms on-chain, i.e. not for off chain signatures?

I think "yes" in both cases. Do you agree?

We don't necessarily know if the user will end up confirming on-chain or off-chain when creating a transaction. As this should be a "hidden / advanced feature" anyways, I think it's not that critical to also have it enabled when the user eventually ends up confirming off-chain though. There's not much downside if someone sets the gas price manually and then signs off chain as this only means the user doesn't have to pay any gas at all & confirmation is instant.

@rmeissner
Copy link
Member

Just to recap (primarily for myself)

There are 2 types of advanced settings:

  1. Safe transaction settings (e.g. Safe transaction nonce, safe tx gas, ...)
  2. Ethereum transaction settings/ Wallet settings (e.g. gas limit, gas price, nonce, ...)

I think first we should define which of these should be able to be set before I can tell you what do differentiate.

For 1. I would only allow to adjust the nonce and the safeTxGas of the Safe transaction. And have the option to set a refund in a later version. But if we allow to specify really all parameter then we definetly need to differentiate between baseGas and safeTxGas (as they have totally different purposes).

For 2. As Lukas said, we actually don't know ahead of time how the confirmation will be created so I would also always display this.

@tschubotz
Copy link
Member

Okay, thank you both for your responses! I updated the ticket description. I think we should start with the following params then in this version:

  • Safe transaction parameters: Safe nonce, safeTxGas
  • Ethereum transaction parameters: Nonce, Gas limit, Gas price

@tschubotz tschubotz changed the title Set transaction parameters manually Set transaction parameters manually (advanced) Nov 5, 2020
@posthnikova
Copy link

I'm for activating it first in advanced settings. This way it doesn't clutter the interface for everyone.

A proposal for sending funds:
https://invis.io/BRZDPI8XPG5

When it's enabled there will be an additional step in send popups. I assume you can leave any field or all fields empty.

@lukasschor
Copy link
Member Author

Activating this in the settings assumes that there are two kinds of users:

  • Users that want to set the parameters manually most of the times
  • Users that don't want to set them manually.

However, I would assume that the lines aren't that clear. And users occasionally would like to set the parameters manually, but most of the time don't want to do so. This is why I would vote for not having to activate the feature in the settings. Also, this would probably make the feature less discoverable and would maybe mean more effort/complexity as there needs to be two different flows depending on whether the setting is activated.

image

Having a simple "advanced options" setting doesn't clutter the interface that much in my opinion.

@tschubotz
Copy link
Member

There could also be a third option, i.e.

  • Option 1: toggle in settings and then there's always that additional step in the tx flow
  • Option 2: No toggle in settings, there's always this "advanced options" thing in the tx flow (so like Metamask)
  • Option 3: Combining the other 2 options by having a toggle in the settings and only then you see the Metamask-like "advanced options" button

@lukasschor
Copy link
Member Author

image

Or always show the transaction details in the review screen and have a "Edit" button to customize. I think that would add value also to users that don't want to set the parameters manually. As even if a user doesn't want to edit the parameters, knowing them before triggering the transaction might be of benefit to users.

@lukasschor
Copy link
Member Author

Also, we should probably also have seperate screens for confirmations/executions. As there, compared to initial transaction creation, the SafeTxGas and Safe nonce cannot be set. Potentially we can still display it there, but show that they are non-modifiable.

@tschubotz
Copy link
Member

Also, we should probably also have seperate screens for confirmations/executions. As there, compared to initial transaction creation, the SafeTxGas and Safe nonce cannot be set. Potentially we can still display it there, but show that they are non-modifiable.

Hm, I generally agree with you, but that's something to check with the devs as they told me that it'll be tricky to figure out if it's an execution/on-chain or off-chain confirmation.

@lukasschor
Copy link
Member Author

I wouldn't try to anticipate whether it's on- or off-chain. I meant that there should be a difference between:

  • Creating a new Tx (Send funds, new contract interaction, new Safe App interaction)
  • Conirming / executing transaction

As with the former category, the user can potentially set the SafeTxGas and Safe nonce, but with the latter, this is not possible anymore.

@tschubotz
Copy link
Member

ah, yes, the safe related items can only be set when creating the tx, not afterwards when confirming / executing.

I was referring to the Ethereum related items.

@posthnikova
Copy link

I have 2 versions, from settings and from Send popup https://invis.io/BRZDPI8XPG5. We don't have data what users prefer, I can include it in the upcoming user test.

Or always show the transaction details in the review screen and have a "Edit" button to customize.
I think that would add value also to users that don't want to set the parameters manually. As even if
a user doesn't want to edit the parameters, knowing them before triggering the transaction might be of
benefit to users.

I wouldn't show parameters the user didn't configure on Review screen. I would offer to edit them earlier.

@tschubotz
Copy link
Member

I like the version with the "Advanced options" button a bit more. But you're right, we don't have data. We just know how Metamask does it.

@illuzen
Copy link

illuzen commented Nov 17, 2020

Hey team, just found this while searching for a way to manually set gas limit, since our transactions (inner, not outer) are failing due to out-of-gas. Is there a workaround while this feature is being developed?

@nicosampler
Copy link
Contributor

safeTxGas attempt of explanation:

It's the amount of gas excectTransaction can use. If the parameter was set to 0, the contract will assume it can spend all remaining gas, if it's > 0 the gas set by the Wallet should be always greater than safeTxGas, otherwise it will fail.
https://github.com/gnosis/safe-contracts/blob/development/contracts/GnosisSafe.sol#L145

@nicosampler
Copy link
Contributor

Regarding Sasha's designs and Lukas's comment #838 (comment), I will implement it so the review screen always includes the details. I believe this information is important.
If in the future the users complain about it, we can modify it so it's only shown if the user clicked on advanced options.

@nicosampler
Copy link
Contributor

@tschubotz @posthnikova @lukasschor
I would like to suggest a change (it was previously mentioned before).

I think it would be better if we remove the "Advanced options" link for the TX screen and show it instead on the review screen.
The main reason for this change is that if the user clicks on "Advanced options" and then in "Confirm" it will be redirected to the "Review screen" and perhaps he didn't finish with the TX parameters (Address, value, etc). Also, it would be kinda weird if in the "review screen" he clicks on "back" and is redirected to the TX parameters instead of "Advanced options".

If you accept this change, the "Advanced options" screen should only have 1 button "confirm". It will help the user to understand that if he did not edit the inputs, the default values will be used.

@tschubotz
Copy link
Member

I like that proposal since it would be similar to what metamask does. However I wouldn't allow the user to confirm a tx right from the advanced parameters screen. I think they should get back to the review screen again where they have to click "confirm"

@posthnikova
Copy link

Initially it was intended that 'Confirm' from Advanced parameters goes back to tx parameters (recipient, amount).

@nicosampler
Copy link
Contributor

nicosampler commented Dec 16, 2020

I think they should get back to the review screen again where they have to click "confirm"

Yes that's what I wanted to say

@tschubotz
Copy link
Member

Initially it was intended that 'Confirm' from Advanced parameters goes back to tx parameters (recipient, amount).

@posthnikova that means though that if they notice on the review page that they want to change something in the advanced params, they would have to

  1. Go back
  2. Open advanced params
  3. Save, so they end up on the tx param screen.
  4. Review

Whereas when we allow them to modify the advanced params from the review screen, that would be way shorter. What do you think about that? Wouldn't that be simpler?

@posthnikova
Copy link

Wouldn't that be simpler?

I think they should get back to the review screen again where they have to click "confirm"

There's a additional click to 'Submit' transaction so the number of clicks is equal. For me both solutions are ok. We don't know how many users would want to configure advanced parameters. Always showing them on Review screen assumes everyone would want to see them.

image

@nicosampler
Copy link
Contributor

nicosampler commented Dec 16, 2020

With how high it is the gas price I bet everyone wants to see it 😝

@nicosampler
Copy link
Contributor

@posthnikova what do you think if in this screen we make "tx details" collapsible?

@posthnikova
Copy link

Do you mean recipient and amount?

@nicosampler
Copy link
Contributor

no I mean this
image

@posthnikova
Copy link

We can do this but the popup would scroll anyway in expanded state.

@nicosampler
Copy link
Contributor

yes, I suggest it as an intermediate alternative to "show it always" or "when the user changed the options".

If we make it collapsible the UI is barely modified and we have the best of the two options. if some users don't want to pay attention to the gas cost they won't expand it.

@posthnikova
Copy link

posthnikova commented Dec 17, 2020

Updated structure with advanced options on Review step.

Send funds, 1 out of x Safe or hardware wallet:

image

Send funds, >1 out of x Safe:

image

Other popups are pretty much the same, parameters you can't set are disabled:

image

Send collectible:

image

Contract interaction:

image

Safe app transaction:

image

@nicosampler
Copy link
Contributor

awesome, love it!

@tschubotz
Copy link
Member

I like this new version as well! Main reasons:

  • Clearly marked as "advacned options"
  • I can review it even though I don't edit it.
  • I see more clearly what can be changed and what cannot.

@posthnikova
Copy link

Zeplin screens:

Send Funds

Contract Interaction

Send Collectible

Safe Apps

Confirmation and execution popups will be later.

@nicosampler
Copy link
Contributor

@rmeissner can you help me to elaborate on a list of validations for the form?

  1. safeTxGas can not be lower than eth gas limit
  2. safeNonce and ethNonce: it can be empty, but if the user enters a number, what should I check? for example if the nonce should be greater than the last one mined.
  3. of course, all values should be > 0.

@rmeissner
Copy link
Member

For 2. Inwould check agaibst on-chain info (or no check atball isbalso fine for me in a first version, up to @tschubotz )
For 3. safeTxGas can be 0 (the nonces too actually)

Aldo this should be added to the issue description ;)

@nicosampler
Copy link
Contributor

the nonces too actually

yes, I wanted to say no negative values

@tschubotz
Copy link
Member

For 2. Inwould check agaibst on-chain info (or no check atball isbalso fine for me in a first version, up to @tschubotz )

Both fine for me too. I think there is no need add more logic. It's an advanced feature, so people should know what they are doing in my opinion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature 👑 New functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants