feat_: integrate base chain #6228
Open
+4,426
−163
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
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
13d8a4e to
b150dc8
Compare
Jenkins BuildsClick to see older builds (192)
|
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
2 times, most recently
from
de357fc to
eff4135
Compare
briansztamfater
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6228 +/- ##
===========================================
+ Coverage 61.55% 61.74% +0.18%
===========================================
Files 842 842
Lines 110604 111256 +652
===========================================
+ Hits 68086 68692 +606
- Misses 34562 34612 +50
+ Partials 7956 7952 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
2 times, most recently
from
e4ef61a to
85a35c3
Compare
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
9 times, most recently
from
c587844 to
514bc34
Compare
briansztamfater
requested review from
alaibe,
shivekkhurana,
vkjr,
dlipicar,
smohamedjavid,
clauxx and
saledjenic
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
514bc34 to
a66b77a
Compare
alaibe
approved these changes
Jan 11, 2025
briansztamfater
force-pushed
the
feat/integrate-base-chain
branch
from
a66b77a to
c294a5e
Compare
saledjenic
reviewed
Jan 12, 2025
There was a problem hiding this comment.
@briansztamfater nice PR. All looks good, could you just update the details I stated in the comments below?
contracts/hop/address.go
Outdated
| L2HopBridgeToken: common.HexToAddress("0xC1985d7a3429cDC85E59E2E4Fcc805b857e6Ee2E"), | ||
| L2AmmWrapper: common.HexToAddress("0x10541b07d8Ad2647Dc6cD67abd4c03575dade261"), | ||
| L2SaddleSwap: common.HexToAddress("0x0ce6c85cF43553DE10FC56cecA0aef6Ff0DD444d"), | ||
| L2SaddleLpToken: common.HexToAddress("0x0ce6c85cF43553DE10FC56cecA0aef6Ff0DD444d"), |
There was a problem hiding this comment.
Suggested change
| L2SaddleLpToken: common.HexToAddress("0x0ce6c85cF43553DE10FC56cecA0aef6Ff0DD444d"), | |
| L2SaddleLpToken: common.HexToAddress("0xe9605BEc1c5C3E81F974F80b8dA9fBEFF4845d4D"), |
| @@ -49,6 +49,8 @@ func getPartnerAddressAndFeePcnt(chainID uint64) (common.Address, float64) { | |||
| return common.HexToAddress("0xE9B59dC0b30cd4646430c25de0111D651c395775"), partnerFeePcnt | |||
| case walletCommon.ArbitrumMainnet: | |||
| return common.HexToAddress("0x9a8278e856C0B191B9daa2d7DD1f7B28268E4DA2"), partnerFeePcnt | |||
| case walletCommon.BaseMainnet: | |||
| return common.Address{}, 0 | |||
There was a problem hiding this comment.
Suggested change
| return common.Address{}, 0 | |
| return common.HexToAddress("0x831be9e08185eba7d88aab1efc059336babef430"), partnerFeePcnt |
There was a problem hiding this comment.
Where did this come from @saledjenic ? AFAIK we still don't have an address for Paraswap partner fees on Base
There was a problem hiding this comment.
Where/how did you find other addresses for other chains?
I was searching for the base chain for exactly the same contract as is used on other chains, and that's the address where it is deployed.
There was a problem hiding this comment.
those are multisig wallets generated by Finance using https://app.safe.global/welcome
There was a problem hiding this comment.
Thanks for pointing this out. I initially left it like this and forgot to find which would be the correct address. Should we ask Finance for the correct address? @dlipicar
There was a problem hiding this comment.
@briansztamfater I've asked Jason from Treasury to generate it and share the address with us, will ping you when I get it! If it takes too long we can just leave it blank and set it in a separate PR.
services/wallet/token/tokenstore.go
Outdated
| @@ -1850,6 +1850,14 @@ func newDefaultStore() *DefaultStore { | |||
| ChainID: 11155420, | |||
| TokenListID: "status", | |||
| }, | |||
| &Token{ | |||
There was a problem hiding this comment.
Do we need this here since we have it in the uniswap.go?
There was a problem hiding this comment.
Also vote for not adding it if it's already in the Uniswap list.
We're gonna need to add SNT/STT here whenever they get deployed to Base, though.
There was a problem hiding this comment.
services/wallet/common/const.go
Outdated
| @@ -110,4 +116,5 @@ var AverageBlockDurationForChain = map[ChainID]time.Duration{ | |||
| ChainID(EthereumMainnet): time.Duration(12000) * time.Millisecond, | |||
| ChainID(OptimismMainnet): time.Duration(400) * time.Millisecond, | |||
| ChainID(ArbitrumMainnet): time.Duration(300) * time.Millisecond, | |||
| ChainID(BaseMainnet): time.Duration(400) * time.Millisecond, | |||
There was a problem hiding this comment.
These values here are incorrect.
Should be:
- OptimismMainnet -> 2 seconds
- Arbitrum -> 0.25 seconds
- Base -> 2 seconds
dlipicar
reviewed
Jan 13, 2025
| @@ -1527,8 +1527,10 @@ func TestWalletConfigOnLoginAccount(t *testing.T) { | |||
| alchemyArbitrumSepoliaToken := "alchemy-arbitrum-sepolia-token" | |||
| alchemyOptimismMainnetToken := "alchemy-optimism-mainnet-token" | |||
| alchemyOptimismSepoliaToken := "alchemy-optimism-sepolia-token" | |||
| raribleMainnetAPIKey := "rarible-mainnet-api-key" // nolint: gosec | |||
| raribleTestnetAPIKey := "rarible-testnet-api-key" // nolint: gosec | |||
| alchemyBaseMainnetToken := "alchemy-base-mainnet-token" // nolint: gosec | |||
There was a problem hiding this comment.
Alchemy is no longer doing this "one key per chain" nonsense (see https://www.alchemy.com/blog/summer-of-chains#simplifying-multichain-development), we should be able to use just a single key for all chains, no need to add more. I guess you can just take any from the map until we rework this? We should now probably have a single alchemy-token variable and remove the per-chain ones.
dlipicar
reviewed
Jan 13, 2025
| @@ -112,7 +112,9 @@ func (t *Transactor) NextNonce(ctx context.Context, rpcClient rpc.ClientInterfac | |||
| // We need to take into consideration all pending transactions in case of Optimism, cause the network returns always | |||
| // the nonce of last executed tx + 1 for the next nonce value. | |||
| if chainID == wallet_common.OptimismMainnet || | |||
| chainID == wallet_common.OptimismSepolia { | |||
| chainID == wallet_common.OptimismSepolia || | |||
| chainID == wallet_common.BaseMainnet || | |||
There was a problem hiding this comment.
hmm @saledjenic any idea about this? If this is correct, please update the comment since it only mentions Optimism.
There was a problem hiding this comment.
Was wandering the same, I hope Brian tested it, cause no other way to figure out this.
There was a problem hiding this comment.
But on the other side, I don't know if we need this at all. I mean, I added that condition a long time ago, but recently (a few months ago) I've faced unreliable nonce resolving on nodefleet for Optimism due to not synced nodes. Since the effect is the same, now I am leaning more toward that we don't need this check. Maybe we should try removing this block entirely and thoroughly inspect sending on Optimism.
@alaibe wdyt?
There was a problem hiding this comment.
Base works the same as Optimism as it is based on the Optimism stack, so if it is a network behavior we should replicate on Base as well. I can update the comment for now, in case this logic is not needed anymore we can remove it in a follow-up PR.
dlipicar
reviewed
Jan 13, 2025
There was a problem hiding this comment.
Rarible supports Base as well: https://docs.rarible.org/docs/supported-chains
We might as well add support there too.
dlipicar
reviewed
Jan 13, 2025
|
@alaibe do we want to support community tokens on Base? I think there's a bunch of changes + deployed contracts missing for that. |
Yeah, I think @0x-r4bbit maintains these contracts and I am not sure anyone can deploy them, but I think we can add support for communities on Base in a separate PR, if needed. |
|
@dlipicar @briansztamfater we just discussed this point, we agreed that this is not a blocker for this PR. |
dlipicar
reviewed
Jan 13, 2025
api/default_networks.go
Outdated
| DefaultFallbackURL: fmt.Sprintf("https://%s.api.status.im/infura/base/mainnet/", stageName), | ||
| DefaultFallbackURL2: fmt.Sprintf("https://%s.api.status.im/grove/base/mainnet/", stageName), | ||
| RPCURL: "https://base-mainnet.infura.io/v3/", | ||
| FallbackURL: "https://base-archival.rpc.grove.city/v1/", |
There was a problem hiding this comment.
there seems to be no base-archival endpoint in Grove, just base
https://docs.grove.city/base-api/intro
dlipicar
reviewed
Jan 13, 2025
api/default_networks.go
Outdated
| DefaultFallbackURL: fmt.Sprintf("https://%s.api.status.im/infura/base/sepolia/", stageName), | ||
| DefaultFallbackURL2: fmt.Sprintf("https://%s.api.status.im/grove/base/sepolia/", stageName), | ||
| RPCURL: "https://base-sepolia.infura.io/v3/", | ||
| FallbackURL: "https://base-sepolia-archival.rpc.grove.city/v1/", |
There was a problem hiding this comment.
No base-sepolia-archival endpoint, just base-testnet
https://docs.grove.city/base-api/intro
dlipicar
force-pushed
the
feat/integrate-base-chain
branch
from
58114fe to
db07128
Compare
Signed-off-by: Brian Sztamfater <[email protected]>
Signed-off-by: Brian Sztamfater <[email protected]>
Signed-off-by: Brian Sztamfater <[email protected]>
dlipicar
force-pushed
the
feat/integrate-base-chain
branch
from
db07128 to
1e4b20e
Compare
dlipicar
reviewed
Jan 14, 2025
| @@ -12,9 +12,11 @@ var contractDataByChainID = map[uint64]common.Address{ | |||
| 1: common.HexToAddress("0x040EA8bFE441597849A9456182fa46D38B75BC05"), // mainnet | |||
| 10: common.HexToAddress("0x55bD303eA3D50FC982A8a5b43972d7f38D129bbF"), // optimism | |||
| 42161: common.HexToAddress("0x54764eF12d29b249fDC7FC3caDc039955A396A8e"), // arbitrum | |||
| 8453: common.HexToAddress("0x84A1C94fcc5EcFA292771f6aE7Fbf24ec062D34e"), // base | |||
There was a problem hiding this comment.
we should stop manually entering chainIDs and just use the wallet constants
dlipicar
reviewed
Jan 14, 2025
| @@ -42,3 +42,4 @@ exclude_patterns: | |||
| - "static/" | |||
| - "t/" | |||
| - "images/qr-assets.go" | |||
| - "contracts/hop/l2Contracts/l2BaseBridge/l2BaseBridge.go" | |||
There was a problem hiding this comment.
hmm @igor-sirotin it seems there's some auto-generated code that's not getting auto-excluded, is there some other place where this needs to be added?
2 tasks
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.
This PR adds support for Base chain.
Closes #6224