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 1276 setup tokamak packages #27

Merged
merged 9 commits into from
Jan 9, 2024
Merged

Conversation

theo-learner
Copy link
Member

@theo-learner theo-learner commented Jan 8, 2024

@theo-learner theo-learner requested a review from a team January 8, 2024 08:45
@theo-learner theo-learner self-assigned this Jan 8, 2024
@@ -0,0 +1,27 @@
// SPDX-License-Identifier: MIT
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 move these files in packages/contracts-bedrock to packages/tokamak/contracts-bedrock. 🙇🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. It's my mistake. Let me fix it.

Copy link
Member Author

@theo-learner theo-learner Jan 9, 2024

Choose a reason for hiding this comment

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

fixed. 3b10012

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 dont really need TON contract here. TON is a deployed smart contract. The current version is also working well with arbitrary ERC-20 token. What we need is ERC-20 addresses.

Copy link
Member

@steven94kr steven94kr Jan 9, 2024

Choose a reason for hiding this comment

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

I agree. How about adding in L2 to use predeploys instead of L1?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure!
It seems that there are similar contracts on packages/contracts-bedrock/src/legacy for predeploy.
Maybe we can add to L2 and if we can use legacy contracts, we will update source code later

Copy link
Member Author

@theo-learner theo-learner Jan 9, 2024

Choose a reason for hiding this comment

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

@rlgns98kr @nguyenzung
You have some confusion due to my lack of background. L1 TON is used for manual deployment in the devnet. We need to provide users with the code to create a devnet environment.

So I think we have to fix the deploy script (packages/tokamak/contracts-bedrock/script/Deploy.s.sol):

  1. you can check the chain id or something so that we can determine if it's a devnet or live network.
    2.1 if we use devnet, we have to deploy TON in L1
    2.2 if we use live network, we should not deploy TON in L1

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!
I understood the context here.
I think using TON contract or ERC20 in oppenzeppelin are all good.
Using our TON contract maybe make user easier for testing since they can add some customized code.
LGTM!

Copy link
Member

@steven94kr steven94kr 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! 🙇🏼‍♂️

@theo-learner theo-learner merged commit 085900f into main Jan 9, 2024
@theo-learner theo-learner deleted the OR-1276_setup-tokamak-packages branch January 9, 2024 08: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.

3 participants