feat: integrate base chain #21876
Merged
feat: integrate base chain #21876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (47)
|
|
@briansztamfater Please use |
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
5 times, most recently
from
94e492f to
b308e82
Compare
briansztamfater
changed the title
briansztamfater
requested review from
alwx,
shivekkhurana,
vkjr,
smohamedjavid,
clauxx and
mohsen-ghafouri
| @@ -135,7 +135,8 @@ | |||
| (def ^:private network-priority-score | |||
| {:ethereum 1 | |||
| :optimism 2 | |||
| :arbitrum 3}) | |||
| :arbitrum 3 | |||
| :base 4}) | |||
There was a problem hiding this comment.
We can move base to rank 2
88% of end-end tests have passedFailed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (7)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
clauxx
reviewed
Jan 15, 2025
There was a problem hiding this comment.
Thanks for the great work!
Left a few comments, but most revolve around minimising the changes needed to add a new chain. Not sure if it's a priority or should be left as a follow-up though @shivekkhurana @ilmotta
Comment on lines
15
to
+17
| :arbitrum {:id 42161 :name "Arbitrum"} | ||
| :optimism {:id 10 :name "Optimism"}}) | ||
| :optimism {:id 10 :name "Optimism"} | ||
| :base {:id 8453 :name "Base"}}) |
There was a problem hiding this comment.
we have these values as constants already
There was a problem hiding this comment.
These data need to be cleaned as it was used by old wallet code
Comment on lines
+34
to
+42
| show-optimism? (and optimism | ||
| (or (pos? (:amount optimism)) | ||
| (= (:amount optimism) "<0.01"))) | ||
| show-arbitrum? (and arbitrum | ||
| (or (pos? (:amount arbitrum)) | ||
| (= (:amount arbitrum) "<0.01"))) | ||
| show-base? (and base | ||
| (or (pos? (:amount base)) | ||
| (= (:amount base) "<0.01")))] |
There was a problem hiding this comment.
It's strange we're adding changes to quo components when adding a new chain, IMO it should be more generic, but it's a larger issue with quo.
For now though, ideally we would want to minimise "chain-specific" code to make it easier to add other ones in the future. This check could be extracted into a function or a separate component, since the operation is the same for each chain.
There was a problem hiding this comment.
I totally agree @clauxx, I think all these checks are not needed anymore as we are showing only one chain at a time, but I'd prefer to refactor in a follow-up as this would be out of scope of this PR.
Comment on lines
+64
to
67
| [network-amount | ||
| {:network :base | ||
| :amount (str (:amount base) " " (or (:token-symbol base) "ETH")) | ||
| :theme theme}])])) |
There was a problem hiding this comment.
The divider? prop seems to be added manually for the chains above and is very easy to miss. Would be better to generalise the logic of when to show the divider instead of manually adding show-base? to all the previous components' divider condition.
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
b308e82 to
285a7a5
Compare
alwx
approved these changes
Jan 15, 2025
smohamedjavid
approved these changes
Jan 16, 2025
Comment on lines
15
to
+17
| :arbitrum {:id 42161 :name "Arbitrum"} | ||
| :optimism {:id 10 :name "Optimism"}}) | ||
| :optimism {:id 10 :name "Optimism"} | ||
| :base {:id 8453 :name "Base"}}) |
There was a problem hiding this comment.
These data need to be cleaned as it was used by old wallet code
clauxx
approved these changes
Jan 16, 2025
82% of end-end tests have passedFailed tests (9)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (46)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestFallbackMultipleDevice:
Class TestActivityMultipleDevicePR:
|
ISSUE 3: "Does not support name" error displayed for sent collectible on Base chainSteps:
Actual result:The "Does not support name" error is shown collectibaleDoesnotsupprt.mp4OS:IOS, Android Devices:
Logshttps://drive.google.com/file/d/1Q9MFtwuFzN2-F2atfOX_m_bR2iuA_1cO/view?usp=drive_link |
|
Thanks for testing @VolodLytvynenko. Issue 3 should be fixed. Investigating the other ones. |
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
3b71c7b to
468193f
Compare
|
Issues 1 and 2 were likely caused by proxy problems (Nodefleet not returning the correct values and other configuration issues). These have now been resolved, so I believe issues 1 and 2 are ready for testing. |
98% of end-end tests have passedExpected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (55)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestFallbackMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
hi @saledjenic. The wrong fee is being displayed on Base. Could you please check this? Perhaps the issue is due to the Status Go PR not being rebased. Could you kindly rebase the Status Go PR? PR_ISSUE 4: The wrong fee is estimated for Base network on send/bridge/swapSteps to reproduce:
Actual result:The estimated maximum fee is always displayed as <0.01 USD.
Expected result:The estimated maximum fee should accurately reflect the actual transaction fee (e.g., 0.1 USD). ADDITIONAL INFOThe same issue happened for the Optimism network |
|
@VolodLytvynenko status-go PR was rebased and updated in this PR @saledjenic Base is based on Optimism stack so if both chains are showing incorrect fees, could it be the case that something changed on counting the L1Fee? |
|
Not sure when this was introduced in status-go but seems L1 Gas Fee is returning 0 value
Based on the tx you attached (https://basescan.org/tx/0xd0347976e2f41cc81c6a64e07650dca0746d24c487cb2e9c56dbaa6d6c0bfedd) The total fes are $0.03
The L2 fee is indeed less than $0.01 (0.000000470854561596 ETH)
Arbitrum works different than Optimism and Base and does not have a separate L1 Fee. For Optimism and Base transactions to show the correct fee we need to receive the correct L1 fee from the router. Seems like something we can fix in a follow up because seems it was not introduced in this PR. |
@briansztamfater thank you. I will create this issue separtly |
|
@briansztamfater Regarding L1 fees, in the past, our 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). I'm not sure if that was due to the calculation method or not, but many months ago, we decided to turn off L1 fees until we checked them again and brought them back. From this point, that wrong value might be coming from unreliable pokt calls/values, so we should prepare a branch including them again and test. I will do that, maybe even today. Also, regarding l1 fee, I see that you're using
|
|
@briansztamfater Thank you for the fixes. Unfortunately, one more issue is found. Perhaps it can be considered a follow-up if no other issue is found ISSUE 5: Bridge transactions to Base fail for ERC-20 assetsSteps:
Actual result:Bridge transactions to Base fail on the blockchain when attempting to bridge an ERC-20 asset. https://arbiscan.io/tx/0xec4ceb7e7ab5a08f1383f3a83e3e734f9dbf20f6eafa8249fc26b090cd9722a0 bridge_tx_base.mp4Expected result:Bridge transactions with ERC-20 assets should complete successfully. Devices:
Logs:https://drive.google.com/file/d/1GuYlSYomppWytPuwCUXfBxcBUynsJozj/view?usp=drive_link |
|
Status of issues I have attached to the description here |
|
ISSUE 4 is not PR related. I've created it separately #21954 |
|
@briansztamfater @VolodLytvynenko here is a PR status-im/status-go#6271 for bringing L1 fees back. Please test it, once changes get integrated in this PR. |
|
hi @briansztamfater no other issues from my side. We have only one issue 5 it's okay if it will be better to fix it in a separate follow-up. Let me know if this will be addressed in this PR, otherwise, the PR can be merged |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
edff672 to
ca954f4
Compare
|
@VolodLytvynenko After some investigation, I realised that bridging DAI to Base is not supported on Hop Protocol. See this screenshot:
This is not the case for USDC, for which I was able to bridge some tokens with success: There is not much that we can do for now until Hop Protocol supports DAI on Base or we integrate a new bridge protocol, but for sure we should prevent the user to execute an invalid bridge transaction. I think the router should not return a valid route in this case, as there are no contract addresses set for Hop Protocol for DAI on Base (https://github.com/hop-protocol/hop/blob/8707bb5fa87c34bef0f98b1a6e2274dde2646fa0/packages/sdk/src/addresses/mainnet.ts#L712). cc @saledjenic Let's merge this and decide what to do in a follow up. Let me know if there will be a new issue for this. |
hi @briansztamfater |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #21529
status go https://github.com/status-im/status-go/tree/feat/integrate-base-chain
Summary
This PR integrates Base chain
Platforms
Areas that maybe impacted
Functional
Steps to test
Send token
Bridge token to Base
Bridge token from Base
Swap tokens on Base
Send collectible
General smoke testing
status: ready
Found Issues
ETH assets and some ERC-20 are not shown for the account on Base
Unable to send ERC-1150 multi-collectible
"Does not support name" error displayed for sent collectible on Base chain
The wrong fee is estimated for Base and Opt networks on send/bridge/swap follow up
Bridge transactions to Base fail for ERC-20 assets