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

[OR-1481] Add code to predeploy USDC and L2UsdcBridge contract in L2 and create genesis file #118

Conversation

xxeonge
Copy link
Collaborator

@xxeonge xxeonge commented Apr 5, 2024

  1. Address allocation for predeploy (addresses.go)
  2. Create binding file about USDC contracts and add library code in foundry.toml
  3. Add constructor and stroges related code to config.go and Immutables.go
  4. Logic updates to proxy to setters.go

In this work, the purpose is to predeploy L2UsdcBridge.sol and FiatTokenV2_2.sol
I added the code necessary to predeploy these two contracts, and if you run make devnet-up, you can see that genesis-l2.json is changed.

+) If L1UsdcBridge is later deployed to L1, the address pre-assigned to L2UsdcBridge have to be changed.

Thank you!! 😄

storage["SignatureChecker"] = state.StorageValues{}
storage["MasterMinter"] = state.StorageValues{
"_owner": config.MasterMinterOwner,
"controllers": controllers,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"controllers": map[any]any{
    "_controller": predeploys.L2UsdcBridgeAddr,
    "_worker":     predeploys.L2UsdcBridgeAddr,
}

How about fix it like this?
Since controllers is only for the L2UsdcBridge, I think it's okay to do this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you good idea. I will fix it!!

@ohbyeongmin
Copy link
Collaborator

ohbyeongmin commented Apr 5, 2024

Is there an option if I don't want to deploy UsdcBridge?
I think provide usdc bridge could be an option when service on-demand L2.
@rlgns98kr @boohyung @praveen263112 What do you think of this?

@rlgns98kr
Copy link
Member

Is there an option if I don't want to deploy UsdcBridge?
I think provide usdc bridge could be an option when service on-demand L2.
@rlgns98kr @boohyung @praveen263112 What do you think of this?

Great!
I think so. I think we need a flag for it as well. 🙇🏻‍♂️

Comment on lines 57 to 60
"tokenName": "Bridged USDC (Tokamak Network)",
"tokenSymbol": "USDC.e",
"tokenCurrency": "USD",
"tokenDecimals": 6,
Copy link
Collaborator

@ohbyeongmin ohbyeongmin Apr 5, 2024

Choose a reason for hiding this comment

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

it looks like common name.
fix to usdcTokenName, usdcTokenSymbol,… or if it's values that doesn't change like WTON, you can put it in directly.

storage["WTON"] = state.StorageValues{

Copy link
Collaborator Author

@xxeonge xxeonge Apr 5, 2024

Choose a reason for hiding this comment

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

Both seem like good ideas. I think We'll have to check later to see if the USDC name can be modified or not!
@rlgns98kr @boohyung

If the name doesn't change, why not use the latter(like WTON)?

Copy link
Member

Choose a reason for hiding this comment

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

It seems great!
Lets remove tokamak network in the token name! And about other values, I think we can fix the value! (we don't have to provide flags!)

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove these flags!
"tokenName": "Bridged USDC (Tokamak Network)", // -> Bridged USDC
"tokenSymbol": "USDC.e",
"tokenCurrency": "USD",
"tokenDecimals": 6,

Copy link
Collaborator Author

@xxeonge xxeonge Apr 5, 2024

Choose a reason for hiding this comment

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

스크린샷 2024-04-05 오후 3 36 21
circle guidelines link:
https://brand.circle.com/d/M9z54TaEwsWL/stablecoins#/-/usdc-brand-guide/usdc-naming-guidelines

circle에서 제공하는 스탠다드에서는 tokenName에 Bridged USDC (company name)을 권장합니다!
토카막에서 배포할 때는 Bridged USDC (Tokamak network)가 스탠다드이지만 온디맨드를 고려했을 때 제 3자가 수정할 수 있게 하기 위해서는 혼란을 방지하고자 Bridged USDC (company name)을 유지하는 게 어떨까 생각합니다.
제 개인적인 생각이라 틀리거나 더 나은 생각이 있다면 말해주시면 감사하겠습니다!🙇‍♀️🙇‍♀️

"tokenSymbol": "USDC.e",
"tokenCurrency": "USD",
"tokenDecimals": 6,

위의 세가지는 WTON과 같이 스토리지를 사용하여 변경되지 않도록하고 tokenName은 회사 이름을 넣을 수 있도록 변경하는 방법도 있을 것 같습니다!


In the standard provided by Circle, it is recommended to use "Bridged USDC (company name)" for the tokenName!

While "Bridged USDC (Tokamak network)" is the standard when deploying on Tokamak, considering on-demand modifications by third parties, to prevent confusion, maintaining "Bridged USDC (company name)" seems reasonable.

This is just my personal opinion, and I would greatly appreciate it if you could let me know if there are any errors or better ideas! 🙇‍♀️🙇‍♀️

The following three, "tokenSymbol": "USDC.e", "tokenCurrency": "USD", "tokenDecimals": 6, could also be maintained using storage like WTON to prevent changes. Additionally, the tokenName could be changed to include the company name.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't know that!
Thank you very much!
It's great!

@nguyenzung
Copy link
Member

nguyenzung commented Apr 5, 2024

Have you tried upgrade those smart contracts? @xxeonge
We are using our own proxies, so maybe we need to check the admin slot for the new Proxies as well.

@xxeonge
Copy link
Collaborator Author

xxeonge commented Apr 5, 2024

You are correct!! Due to issues with the proxies we're using, so I considered upgrading the contracts.
However, I've been advised against upgrading the USDC and L2UsdcBridge contracts hastily, as they conform to the Circle standard. So, instead, we've updated the proxy logic for the USDC contract in setters.go, with assistance from Steven. Thank you!😊 @nguyenzung

Copy link
Member

@rlgns98kr rlgns98kr left a comment

Choose a reason for hiding this comment

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

Thank you very much! 🙇🏼‍♂️

@nguyenzung
Copy link
Member

nguyenzung commented Apr 8, 2024

You are correct!! Due to issues with the proxies we're using, so I considered upgrading the contracts. However, I've been advised against upgrading the USDC and L2UsdcBridge contracts hastily, as they conform to the Circle standard. So, instead, we've updated the proxy logic for the USDC contract in setters.go, with assistance from Steven. Thank you!😊 @nguyenzung

Thank you
I think it looks good right now.
I think we can check this line later to make sure it can work with our own proxy, because the slot for our proxies can be different than the slot for Optimism's Proxy : https://github.com/tokamak-network/tokamak-thanos/blob/OR-1481-Add-some-USDC.e-and-USDC-Bridge-code-to-create-genesis-file/op-chain-ops/genesis/setters.go#L77

@xxeonge xxeonge merged commit 9eea985 into main Apr 8, 2024
6 checks passed
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.

4 participants