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

chore(wallet)_: include l1 fees info (for the tx and approval) in the response #6271

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

saledjenic
Copy link
Contributor

A while ago we disabled l1 info cause the estimated L1 fees were significantly higher than they should have been (if I remember well, the L1 fees were about 1000 times higher than they should have been).

From this point, that wrong value might be coming from unreliable pokt calls/values, these changes bring them back.

@saledjenic
Copy link
Contributor Author

Initiative for this PR is coming from this comment status-im/status-mobile#21876 (comment)

@status-im-auto
Copy link
Member

status-im-auto commented Jan 20, 2025

Jenkins Builds

Click to see older builds (19)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 67b8da3 #1 2025-01-20 10:38:45 ~3 min ios 📦zip
✔️ 67b8da3 #1 2025-01-20 10:40:17 ~5 min linux 📦zip
✔️ 67b8da3 #1 2025-01-20 10:40:27 ~5 min macos 📦zip
✔️ 67b8da3 #1 2025-01-20 10:40:36 ~5 min android 📦aar
✔️ 67b8da3 #1 2025-01-20 10:40:45 ~5 min macos 📦zip
✖️ 67b8da3 #1 2025-01-20 10:40:49 ~5 min tests-rpc 📄log
✔️ 67b8da3 #1 2025-01-20 10:41:04 ~6 min windows 📦zip
✖️ 67b8da3 #1 2025-01-20 11:05:34 ~30 min tests 📄log
✔️ 214e425 #2 2025-01-23 18:27:23 ~5 min ios 📦zip
✔️ 214e425 #2 2025-01-23 18:59:37 ~6 min android 📦aar
✖️ 214e425 #2 2025-01-23 21:33:14 ~31 min tests 📄log
✖️ 214e425 #2 2025-01-23 23:20:05 ~5 min tests-rpc 📄log
✔️ 214e425 #2 2025-01-24 10:47:51 ~5 min linux 📦zip
✔️ 214e425 #2 2025-01-24 11:21:03 ~5 min macos 📦zip
✔️ 214e425 #2 2025-01-24 11:26:09 ~7 min macos 📦zip
✔️ 214e425 #2 2025-01-24 13:05:48 ~7 min windows 📦zip
✖️ 214e425 #3 2025-01-24 13:05:58 ~3 min tests-rpc 📄log
✖️ 214e425 #4 2025-01-24 13:23:57 ~3 min tests-rpc 📄log
✖️ 214e425 #3 2025-01-24 13:49:36 ~29 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 39ed123 #3 2025-01-24 13:43:59 ~3 min macos 📦zip
✔️ 39ed123 #3 2025-01-24 13:44:15 ~4 min ios 📦zip
✔️ 39ed123 #3 2025-01-24 13:44:45 ~4 min windows 📦zip
✔️ 39ed123 #3 2025-01-24 13:45:30 ~5 min macos 📦zip
✔️ 39ed123 #3 2025-01-24 13:45:36 ~5 min linux 📦zip
✔️ 39ed123 #3 2025-01-24 13:45:43 ~5 min android 📦aar
✖️ 39ed123 #5 2025-01-24 13:45:52 ~5 min tests-rpc 📄log
✖️ 39ed123 #4 2025-01-24 14:20:19 ~30 min tests 📄log
✔️ 3a81b49 #4 2025-01-24 15:12:49 ~3 min ios 📦zip
✔️ 3a81b49 #4 2025-01-24 15:12:58 ~3 min macos 📦zip
✔️ 3a81b49 #4 2025-01-24 15:13:17 ~4 min windows 📦zip
✔️ 3a81b49 #4 2025-01-24 15:14:26 ~5 min macos 📦zip
✔️ 3a81b49 #4 2025-01-24 15:14:37 ~5 min linux 📦zip
✖️ 3a81b49 #6 2025-01-24 15:14:38 ~5 min tests-rpc 📄log
✔️ 3a81b49 #4 2025-01-24 15:14:47 ~5 min android 📦aar
✔️ 3a81b49 #5 2025-01-24 15:40:09 ~31 min tests 📄log

Copy link
Collaborator

@alaibe alaibe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have u confirmed the value makes sense?

@saledjenic
Copy link
Contributor Author

Have u confirmed the value makes sense?

I did not. Expect people from the mobile team to check and respond to that.

@pavloburykh
Copy link

@saledjenic thank you for the PR. Could you please check the ISSUE 1 identified in corresponding mobile PR?

@saledjenic saledjenic force-pushed the chore/include-l1-fees branch from 67b8da3 to 214e425 Compare January 23, 2025 14:28
@saledjenic
Copy link
Contributor Author

@pavloburykh could you try again now?

Copy link
Contributor

@dlipicar dlipicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good, waiting for confirmation that the values actually make sense

@pavloburykh
Copy link

@pavloburykh could you try again now?

thank you @saledjenic! @VolodLytvynenko will be able to pick it up tomorrow, as I will be off. Also, I see some builds failed in this PR, is it related to changes of some infra issues?

@saledjenic
Copy link
Contributor Author

is it related to changes of some infra issues?

Should not be up to the changes here, something must be stuck, will push again later and see.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 55.88235% with 30 lines in your changes missing coverage. Please review.

Project coverage is 61.40%. Comparing base (e71fdbc) to head (3a81b49).

Files with missing lines Patch % Lines
services/wallet/router/router.go 50.00% 7 Missing and 1 partial ⚠️
services/wallet/router/router_helper.go 79.31% 5 Missing and 1 partial ⚠️
services/wallet/router/routes/router_path.go 0.00% 4 Missing and 2 partials ⚠️
...vices/wallet/transfer/transaction_manager_route.go 0.00% 2 Missing ⚠️
...let/router/pathprocessor/processor_bridge_celar.go 0.00% 1 Missing ⚠️
...allet/router/pathprocessor/processor_bridge_hop.go 75.00% 1 Missing ⚠️
...t/router/pathprocessor/processor_ens_public_key.go 0.00% 1 Missing ⚠️
...let/router/pathprocessor/processor_ens_register.go 0.00% 1 Missing ⚠️
...llet/router/pathprocessor/processor_ens_release.go 0.00% 1 Missing ⚠️
...s/wallet/router/pathprocessor/processor_erc1155.go 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6271      +/-   ##
===========================================
- Coverage    61.84%   61.40%   -0.45%     
===========================================
  Files          843      843              
  Lines       111287   111255      -32     
===========================================
- Hits         68824    68312     -512     
- Misses       34497    35025     +528     
+ Partials      7966     7918      -48     
Flag Coverage Δ
functional 20.46% <33.82%> (-1.09%) ⬇️
unit 60.35% <45.58%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...et/router/pathprocessor/processor_swap_paraswap.go 48.73% <100.00%> (+1.01%) ⬆️
.../wallet/router/pathprocessor/processor_transfer.go 56.92% <100.00%> (-0.66%) ⬇️
...let/router/pathprocessor/processor_bridge_celar.go 0.00% <0.00%> (ø)
...allet/router/pathprocessor/processor_bridge_hop.go 11.82% <75.00%> (+0.64%) ⬆️
...t/router/pathprocessor/processor_ens_public_key.go 15.51% <0.00%> (+0.76%) ⬆️
...let/router/pathprocessor/processor_ens_register.go 12.16% <0.00%> (+0.47%) ⬆️
...llet/router/pathprocessor/processor_ens_release.go 15.51% <0.00%> (+0.76%) ⬆️
...s/wallet/router/pathprocessor/processor_erc1155.go 4.49% <0.00%> (+0.14%) ⬆️
...es/wallet/router/pathprocessor/processor_erc721.go 3.27% <0.00%> (+0.07%) ⬆️
...let/router/pathprocessor/processor_stickers_buy.go 11.94% <0.00%> (+0.51%) ⬆️
... and 4 more

... and 47 files with indirect coverage changes

@saledjenic saledjenic force-pushed the chore/include-l1-fees branch from 214e425 to 39ed123 Compare January 24, 2025 13:39
@saledjenic
Copy link
Contributor Author

@antdanchenko could you please help on this, cause functional tests must be failing due to missing OVM_GasPriceOracle contract on Anvil?

… response

While ago we've disabled l1 info cause the estimated L1 fees were significantly higher than they should have been
(if I remember well, the L1 fees were about 1000 times higher than they should have been).

From this point, that wrong value might be coming from unreliable pokt calls/values, these changes bring them back.
@saledjenic saledjenic force-pushed the chore/include-l1-fees branch from 39ed123 to 3a81b49 Compare January 24, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants