-
Notifications
You must be signed in to change notification settings - Fork 250
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
Conversation
Initiative for this PR is coming from this comment status-im/status-mobile#21876 (comment) |
Jenkins BuildsClick to see older builds (77)
|
There was a problem hiding this 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?
I did not. Expect people from the mobile team to check and respond to that. |
@saledjenic thank you for the PR. Could you please check the ISSUE 1 identified in corresponding mobile PR? |
67b8da3
to
214e425
Compare
@pavloburykh could you try again now? |
There was a problem hiding this 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
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? |
Should not be up to the changes here, something must be stuck, will push again later and see. |
214e425
to
39ed123
Compare
@antdanchenko could you please help on this, cause functional tests must be failing due to missing |
39ed123
to
3a81b49
Compare
Thanks @saledjenic. Could you please take a look at the ISSUE 2 reported by @VolodLytvynenko? |
3a81b49
to
b554ae2
Compare
@pavloburykh all that should be already addressed, could you test the mobile app pointing to the most recent changes in this PR? |
|
I made some comparisons across chains to see how much the values in the Status app differed from the calculations received by Infura's Here are the findings:Mainnet(1):
Optimism(10):
Arbitrum(42161):
|
Please have a look at the second commit of this PR. I've spent some time adjusting the fees we calculate in the Status app and was trying to make them close to the values displayed in Metamask. Some logic for achieving that is added to the second commit, our fees are slightly higher, but that's ok, I guess. Logic:
Mainnet(1)
Optimism(10)
Arbitrum(42161)
|
avgGasUsedRatio := totalGasUsedRatio / float64(len(feeHistory.GasUsedRatio)) | ||
|
||
priorityWeight := 0.7 | ||
gasUsedWeight := 0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe const?
Also I guess writing comment about what they are would be useful for future code reader
f126502
to
c9651fa
Compare
@ALL @dlipicar @friofry @briansztamfater could you review the second commit, that should bring us closer to metamask values? |
phew! Alright, looks like we're closer. |
72ac3ad
to
6878af5
Compare
… 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.
6878af5
to
e0ac920
Compare
…estimating l1 fee - A new contract is used for estimating L1 fee. - New contract's addresses are known only for Optimims and Sepolia Optimimsm. - Old contract and addresses per chain are kept so far, but not in use anymore. Comparing to other wallets, seems L1 fee is added to the total fee only for the Optimims chain. So we're doing the same in this commit, disabling L1 fees for other than the Optimism.
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.