feat(AmountToSend): refactor to use exact amounts for transactions #12169

micieslak · 2023-09-15T09:26:07Z

What does the PR do

Fixes inaccuracies caused by passing amount to be transferred as a float from UI to backend.

transfer method transaction/service.nim takes amount in base units (integer) instead as floats (base units divided by decimals parameter, 18 for eth). Thanks to that eth2Wei (leading to problems described in [EPIC] Exchange amounts between UI and backend using strings with base units instead of js numbers #11376) is not used internally. UI is responsible to provide exact amount to transfer.
rawBalance exposed to UI as decimal integer string (token model)
AmountToSend refactored/simplified
base unit tests for LocaleUtils::currencyAmountToLocaleString

Closes: #12168

Affected areas

SendModal, AmountToSend, backend: transaction service

Screenshot of functionality (including design for comparison)

Kazam_screencast_00031.webm

Before the change, the same transaction for 0.07 would end up with following transaction:

status-im-auto · 2023-09-15T09:32:02Z

Jenkins Builds

Click to see older builds (42)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`ce3050a`	#1	2023-09-15 09:32:02	~5 min	`tests/nim`	📄`log`
✔️	`ce3050a`	#1	2023-09-15 09:32:53	~6 min	`tests/statusq`	📄`log`
✔️	`ce3050a`	#1	2023-09-15 09:40:50	~14 min	`macos/x86_64`	🍎`dmg`
✔️	`ce3050a`	#1	2023-09-15 09:42:03	~15 min	`linux/x86_64`	📦`tgz`
✖️	`ce3050a`	#1	2023-09-15 09:57:15	~30 min	`tests/e2e`	📄`log`
✔️	`ce3050a`	#1	2023-09-15 09:57:57	~31 min	`windows/x86_64`	💿`exe`

✔️	`39cbe4d`	#2	2023-09-15 19:49:04	~5 min	`tests/statusq`	📄`log`
✔️	`39cbe4d`	#2	2023-09-15 19:49:53	~6 min	`tests/nim`	📄`log`
✔️	`39cbe4d`	#2	2023-09-15 19:52:20	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`39cbe4d`	#2	2023-09-15 19:58:21	~14 min	`linux/x86_64`	📦`tgz`
✔️	`39cbe4d`	#2	2023-09-15 20:09:36	~25 min	`windows/x86_64`	💿`exe`
✔️	`39cbe4d`	#2	2023-09-15 20:16:46	~32 min	`tests/e2e`	📄`log`

✔️	`451f439`	#3	2023-09-15 20:26:16	~5 min	`tests/nim`	📄`log`
✔️	`451f439`	#3	2023-09-15 20:26:19	~5 min	`macos/aarch64`	🍎`dmg`
✔️	`451f439`	#3	2023-09-15 20:26:38	~5 min	`tests/statusq`	📄`log`
✔️	`451f439`	#3	2023-09-15 20:28:12	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`451f439`	#3	2023-09-15 20:35:38	~14 min	`linux/x86_64`	📦`tgz`
✔️	`451f439`	#3	2023-09-15 20:42:04	~21 min	`windows/x86_64`	💿`exe`
✔️	`451f439`	#3	2023-09-15 20:52:07	~31 min	`tests/e2e`	📄`log`

✔️	`c692a58`	#4	2023-09-15 21:34:34	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`c692a58`	#4	2023-09-15 21:35:20	~5 min	`tests/nim`	📄`log`
✔️	`c692a58`	#4	2023-09-15 21:35:35	~5 min	`tests/statusq`	📄`log`
✔️	`c692a58`	#4	2023-09-15 21:37:27	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`c692a58`	#4	2023-09-15 21:43:43	~13 min	`linux/x86_64`	📦`tgz`
✔️	`c692a58`	#4	2023-09-15 21:52:43	~22 min	`windows/x86_64`	💿`exe`
✔️	`c692a58`	#4	2023-09-15 22:00:10	~30 min	`tests/e2e`	📄`log`
✖️	`c692a58`	#5	2023-09-17 07:25:30	~7 min	`tests/e2e`	📄`log`
✔️	`c692a58`	#6	2023-09-17 07:59:09	~28 min	`tests/e2e`	📄`log`

✔️	`e32a372`	#6	2023-09-20 09:21:18	~4 min	`tests/statusq`	📄`log`
✔️	`e32a372`	#6	2023-09-20 09:21:56	~4 min	`tests/nim`	📄`log`
✔️	`e32a372`	#6	2023-09-20 09:22:22	~5 min	`macos/aarch64`	🍎`dmg`
✔️	`e32a372`	#6	2023-09-20 09:30:38	~13 min	`macos/x86_64`	🍎`dmg`
✔️	`e32a372`	#6	2023-09-20 09:30:45	~13 min	`linux/x86_64`	📦`tgz`
✔️	`e32a372`	#6	2023-09-20 09:44:15	~27 min	`windows/x86_64`	💿`exe`
✖️	`e32a372`	#8	2023-09-20 09:49:28	~32 min	`tests/e2e`	📄`log`

✔️	`5ceb2ed`	#7	2023-09-20 10:09:11	~5 min	`tests/statusq`	📄`log`
✔️	`5ceb2ed`	#7	2023-09-20 10:09:17	~5 min	`tests/nim`	📄`log`
✔️	`5ceb2ed`	#7	2023-09-20 10:10:42	~7 min	`macos/aarch64`	🍎`dmg`
✔️	`5ceb2ed`	#7	2023-09-20 10:12:14	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`5ceb2ed`	#7	2023-09-20 10:17:11	~13 min	`linux/x86_64`	📦`tgz`
✔️	`5ceb2ed`	#7	2023-09-20 10:27:07	~23 min	`windows/x86_64`	💿`exe`
✖️	`5ceb2ed`	#10	2023-09-20 10:33:57	~30 min	`tests/e2e`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`0448918`	#8	2023-09-20 10:54:36	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`0448918`	#8	2023-09-20 10:55:10	~5 min	`tests/statusq`	📄`log`
✔️	`0448918`	#8	2023-09-20 10:55:46	~6 min	`tests/nim`	📄`log`
✔️	`0448918`	#8	2023-09-20 10:57:04	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`0448918`	#8	2023-09-20 11:03:07	~13 min	`linux/x86_64`	📦`tgz`
✔️	`0448918`	#8	2023-09-20 11:14:15	~24 min	`windows/x86_64`	💿`exe`
✔️	`0448918`	#11	2023-09-20 11:20:42	~31 min	`tests/e2e`	📄`log`

✔️	`4180b56`	#9	2023-09-20 12:47:20	~5 min	`macos/aarch64`	🍎`dmg`
✔️	`4180b56`	#9	2023-09-20 12:47:37	~5 min	`tests/statusq`	📄`log`
✔️	`4180b56`	#9	2023-09-20 12:48:11	~6 min	`tests/nim`	📄`log`
✔️	`4180b56`	#9	2023-09-20 12:50:07	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`4180b56`	#9	2023-09-20 12:55:54	~14 min	`linux/x86_64`	📦`tgz`
✔️	`4180b56`	#9	2023-09-20 13:06:36	~24 min	`windows/x86_64`	💿`exe`
✔️	`4180b56`	#12	2023-09-20 13:11:36	~29 min	`tests/e2e`	📄`log`

alexjba

LGTM in general.
One question though: Do you know what generates the difference between Amount to send and Received amount? I'm sending 0.01ETH and the receiver will receive more.

storybook/pages/AmountToSendPage.qml

caybro

LGTM

Minus the objection/question from @alexjba

micieslak · 2023-09-20T09:07:30Z

LGTM in general. One question though: Do you know what generates the difference between Amount to send and Received amount? I'm sending 0.01ETH and the receiver will receive more.

It looks strange indeed, also the max fees value which is negative in your case. But I cannot reproduce that, for me it behaves as expected.

Is it easily reproducible in your env? What about master, it works the same way? I looks like unrelated with my changes but it would be nice to check it.

caybro · 2023-09-20T09:08:29Z

LGTM in general. One question though: Do you know what generates the difference between Amount to send and Received amount? I'm sending 0.01ETH and the receiver will receive more.

It looks strange indeed, also the max fees value which is negative in your case. But I cannot reproduce that, for me it behaves as expected.

Is it easily reproducible in your env? What about master, it works the same way? I looks like unrelated with my changes but it would be nice to check it.

Mainnet vs. optimism?

Khushboo-dev-cpp · 2023-09-20T09:29:59Z

LGTM in general. One question though: Do you know what generates the difference between Amount to send and Received amount? I'm sending 0.01ETH and the receiver will receive more.

It looks strange indeed, also the max fees value which is negative in your case. But I cannot reproduce that, for me it behaves as expected.

Is it easily reproducible in your env? What about master, it works the same way? I looks like unrelated with my changes but it would be nice to check it.

Mainnet vs. optimism?

Hey the fact that you are receiving more than you send is something that can only happen on testnet, and this because that traffic is low and this is an incentive to send a particular token to that particular network

alexjba · 2023-09-20T09:42:34Z

Hey the fact that you are receiving more than you send is something that can only happen on testnet, and this because that traffic is low and this is an incentive to send a particular token to that particular network

Thanks @Khushboo-dev-cpp! Had no idea we can have valid negative fees.

Khushboo-dev-cpp · 2023-09-20T11:13:49Z

ui/imports/shared/popups/send/SendModal.qml

                            isBridgeTx: d.isBridgeTx
                            interactive: popup.interactive
                            selectedSymbol: d.selectedSymbol
                            maxInputBalance: d.maxInputBalance
                            currentCurrency: popup.currencyStore.currentCurrency
+


will the multiplier by default be 1 or 0 ?

multiplierIndex default value should be 0 bc it means 10^0, effectively multiplier is 1

Khushboo-dev-cpp

Awesome work @micieslak ! Looks good to me :)

alaibe · 2023-09-20T11:34:01Z

src/app_service/service/transaction/service.nim

@@ -261,7 +261,7 @@ QtObject:
  ) =
    try:
      var paths: seq[TransactionBridgeDto] = @[]
-      let amountToSend = conversion.eth2Wei(parseFloat(value), 18)
+      let amountToSend = value.parse(Uint256)


does that require change in status go?

For this change - no. In general taking raw amount from go is needed to e.g. reliably compare values

Closes: #12168

micieslak marked this pull request as draft September 15, 2023 09:26

micieslak force-pushed the chore/issue-12168 branch 2 times, most recently from 39cbe4d to 451f439 Compare September 15, 2023 20:20

micieslak requested review from dlipicar, Khushboo-dev-cpp, alaibe, caybro and alexjba September 15, 2023 20:56

micieslak force-pushed the chore/issue-12168 branch from 451f439 to c692a58 Compare September 15, 2023 21:29

micieslak marked this pull request as ready for review September 15, 2023 21:30

alexjba reviewed Sep 17, 2023

View reviewed changes

storybook/pages/AmountToSendPage.qml Outdated Show resolved Hide resolved

micieslak force-pushed the chore/issue-12168 branch from c692a58 to 57eecb4 Compare September 20, 2023 08:46

caybro approved these changes Sep 20, 2023

View reviewed changes

micieslak force-pushed the chore/issue-12168 branch from 57eecb4 to e32a372 Compare September 20, 2023 09:16

alexjba approved these changes Sep 20, 2023

View reviewed changes

micieslak force-pushed the chore/issue-12168 branch 2 times, most recently from 5ceb2ed to 0448918 Compare September 20, 2023 10:49

Khushboo-dev-cpp reviewed Sep 20, 2023

View reviewed changes

Khushboo-dev-cpp approved these changes Sep 20, 2023

View reviewed changes

alaibe reviewed Sep 20, 2023

View reviewed changes

alaibe approved these changes Sep 20, 2023

View reviewed changes

micieslak added 2 commits September 20, 2023 14:41

StatusQ(LocaleUitls): Unit test added for currencyAmountToLocaleString

c07230d

feat: Refactor AmountToSend to use exact amounts for transactions

4180b56

Closes: #12168

micieslak force-pushed the chore/issue-12168 branch from 0448918 to 4180b56 Compare September 20, 2023 12:41

micieslak mentioned this pull request Sep 20, 2023

Refactor NetworksAdvancedCustomRoutingView component to use exact amounts for transactions #12217

Open

micieslak merged commit fb48e7b into master Sep 20, 2023
8 checks passed

micieslak deleted the chore/issue-12168 branch September 20, 2023 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AmountToSend): refactor to use exact amounts for transactions #12169

feat(AmountToSend): refactor to use exact amounts for transactions #12169

micieslak commented Sep 15, 2023 •

edited

Loading

status-im-auto commented Sep 15, 2023 •

edited

Loading

alexjba left a comment

caybro left a comment

micieslak commented Sep 20, 2023

caybro commented Sep 20, 2023

Khushboo-dev-cpp commented Sep 20, 2023

alexjba commented Sep 20, 2023

Khushboo-dev-cpp Sep 20, 2023

micieslak Sep 20, 2023

Khushboo-dev-cpp left a comment

alaibe Sep 20, 2023

micieslak Sep 20, 2023

feat(AmountToSend): refactor to use exact amounts for transactions #12169

feat(AmountToSend): refactor to use exact amounts for transactions #12169

Conversation

micieslak commented Sep 15, 2023 • edited Loading

What does the PR do

Affected areas

Screenshot of functionality (including design for comparison)

status-im-auto commented Sep 15, 2023 • edited Loading

Jenkins Builds

alexjba left a comment

Choose a reason for hiding this comment

caybro left a comment

Choose a reason for hiding this comment

micieslak commented Sep 20, 2023

caybro commented Sep 20, 2023

Khushboo-dev-cpp commented Sep 20, 2023

alexjba commented Sep 20, 2023

Khushboo-dev-cpp Sep 20, 2023

Choose a reason for hiding this comment

micieslak Sep 20, 2023

Choose a reason for hiding this comment

Khushboo-dev-cpp left a comment

Choose a reason for hiding this comment

alaibe Sep 20, 2023

Choose a reason for hiding this comment

micieslak Sep 20, 2023

Choose a reason for hiding this comment

micieslak commented Sep 15, 2023 •

edited

Loading

status-im-auto commented Sep 15, 2023 •

edited

Loading