Safe-react: Spending limit - Editing current spending limit inconsistency in the UI

[francovenica]

Issue:
When an owner used an X amount of his spending limit and then it is edited, allowing him an amount lower than what he already used

Steps:
1 - Allow an owner 1 of a token (ETH for example)
2 - With the owner use the 0.5 ETH
3 - Edit the Spending limit, from 1ETH to 0.25 ETH
4 - Go to the send funds form and check the spending limit check button

[francovenica]

@tschubotz @fernandomg
For this one I'm not sure what we could do:
1 - Simply tell him that he has reach the limit,
2 - Grey the Spending limit check button so he is not even able to use the option
3 - A message telling the owner to request more tokens

What do you think?

[fernandomg]

I'll go with option 2, disabling the spending limit in the "Send Funds" modal.

To prevent this scenario in the UI we may need to first delete the spending limit that existed, and then set the new one. This can be done with a multiSend transaction.

[tschubotz]

@fernandomg
So even though we write "you are about to replace ..", the spending limit doesn't get replaced, but only modified? I would have expected that when the UI says "spending limit gets replaced", it starts from 0 again 🤔

[fernandomg]

So even though we write "you are about to replace ..", the spending limit doesn't get replaced, but only modified? I would have expected that when the UI says "spending limit gets replaced", it starts from 0 again 🤔

Yes, so did I.

I thought that by _setting_ we were replacing but it was _updating_ it instead.

WDYT about separating this into another issue?

[tschubotz]

Yea, I was just reading through the contract code. It's indeed updating: 0xcfbfac74c26f8647cbdb8c5caf80bb5b32e43134

So in order to actually replace it, we would have to first delete the previous allowance.

And yes, let's go with (2) for this one. And let's split the replacing into another issue so we can separately decide if it's necessary or not.

[fernandomg]

@tschubotz,

And let's split the replacing into another issue so we can separately decide if it's necessary or not.

Just created the #1700