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

feat(AssetMigration): add implement for AssetMigration and FunctionRestrictable #88

Open
wants to merge 52 commits into
base: testnet
Choose a base branch
from

Conversation

TuDo1403
Copy link
Collaborator

@TuDo1403 TuDo1403 commented Dec 5, 2024

Description

This PR implements:

AssetMigration Module

  • migrateERC20: migrate specific amount of ERC20 tokens to whitelisted address.

** note **: native will be wrapped into WETH, therefore while whitelisting, ensure to whitelist wrapped ERC20 address, instead of address(0).

FunctionRestrictable Module

  • restrict(bytes4, uint8): allow pause specific function with specific token standard.

Checklist

  • I have clearly commented on all the main functions following the NatSpec Format
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works

@TuDo1403 TuDo1403 force-pushed the feature/asset-migration branch from c7e7e72 to d1ae509 Compare December 5, 2024 06:59
@TuDo1403 TuDo1403 force-pushed the feature/asset-migration branch from d1ae509 to 0330d4f Compare December 5, 2024 07:04
Copy link
Collaborator

@huyhuynh3103 huyhuynh3103 left a comment

Choose a reason for hiding this comment

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

Wrong storage slot

Copy link
Collaborator

@huyhuynh3103 huyhuynh3103 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@thaixuandang thaixuandang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@huyhuynh3103 huyhuynh3103 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@minh-bq minh-bq left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me but unit tests need to be added.

@@ -58,81 +61,16 @@ contract MainchainGatewayV3 is
_disableInitializers();
}

fallback() external payable {

Choose a reason for hiding this comment

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

Any requirement for this? Otherwise should not remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

an attempt to reduce code size, we also do not need this function too.

Choose a reason for hiding this comment

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

I think it's not necessary (just a few bytes) and might break things. Please do not remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this function is kept, the current restrict logic can be bypassed using this function to create a deposit when deposit is restricted.

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.

5 participants