Concordium / concordium-reference-wallet-ios

Reference wallet for the Concordium blockchain
Apache License 2.0
11 stars 10 forks source link

Contract update failed insufficient energy fix #391

Closed ramakser closed 7 months ago

ramakser commented 7 months ago

Purpose

Estimation transaction fee is not always the same for the same transaction. Fix CIS2 token look list image

Changes

Fix transaction fee update logic while user input amount of token to send. Replaced SDWebImage with Async image due to hight memory consumption during list tokens scroll

bisgardo commented 7 months ago

It's not clear to me what exact bug this is fixing. I understand that debounding the cost estimation requests makes it more likely that there's only one request in flight at a time. Was the problem before that responses might arrive out of order? If so, I think this helps but doesn't fix it. And not sending a request if there's already one in flight is wrong.

The correct solution I think is to do the debounce but always send the request. Either cancel any existing requests or ignore their response (for example pass a counter or timestamp though the pipeline and check it against the component state).

ramakser commented 7 months ago

It's not clear to me what exact bug this is fixing. I understand that debounding the cost estimation requests makes it more likely that there's only one request in flight at a time. Was the problem before that responses might arrive out of order? If so, I think this helps but doesn't fix it. And not sending a request if there's already one in flight is wrong.

The correct solution I think is to do the debounce but always send the request. Either cancel any existing requests or ignore their response (for example pass a counter or timestamp though the pipeline and check it against the component state).

we need to disable send button till we get correct tx transaction cost, otherwise, it will fails transaction at all, like it happens now

ramakser commented 7 months ago

I think this solution still doesn't take into account that cost estimations may return out of order.

And there probably also is the edge case that because of the debounc delay, there is a window from the last input was entered until the estimation request is sent where the button is enabled.

I'll approve as this solution is an improvement over what's there now.

there is a window from the last input was entered until the estimation request is sent where the button is enabled. good point, will try to find solution to handle this

bisgardo commented 7 months ago

will try to find solution to handle this

A simple option is to set isUpdatingTransferCost = true before the debounce. I.e. something like:

        view.amountTextPublisher
            .map { [weak self] val in
                self?.isUpdatingTransferCost = true
                return val
            }
            .debounce(for: 0.7, scheduler: DispatchQueue.main)
            ...

I belive this should work because you know that updateTransferCostEstimate will always be called after this.