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.

Sign up for GitHub

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

Jump to bottom

feat!: external erc20 support #1234

Merged
merged 3 commits into from
Jan 27, 2022
Merged

feat!: external erc20 support #1234

merged 3 commits into from
Jan 27, 2022

Conversation

fish-sammy
Copy link
Contributor

@fish-sammy fish-sammy commented Jan 18, 2022 •
edited

Description

Mint and Burn worked at
https://ropsten.etherscan.io/tx/0xb7a66f34db2101f130db03b24058fa076039242512fbd4c5af64099cb076c1c2
https://ropsten.etherscan.io/tx/0x50a06b02579c363c9562bf233484530a359b697128e96e37f3ea49c2c90f9d6f

contracts' bytecode matches axelarnetwork/axelar-cgp-solidity#50

Todos

  • Unit tests
  • Manual tests
  • Documentation
  • Connect epics/issues
  • Tag type of change

Steps to Test

Expected Behaviour

Other Notes

@fish-sammy fish-sammy added the enhancement New feature or request label Jan 18, 2022
@fish-sammy fish-sammy linked an issue Jan 18, 2022 that may be closed by this pull request
external erc-20 #1231
Closed
@fish-sammy fish-sammy force-pushed the feat/external-erc20-support branch 2 times, most recently from fa8a3fa to ec3a450 Compare January 19, 2022 17:18
jcs47
jcs47 reviewed Jan 19, 2022
View reviewed changes
x/evm/client/rest/tx.go Show resolved Hide resolved
x/evm/keeper/msg_server.go
Comment on lines +1024 to +1029
if !s.nexus.IsAssetRegistered(ctx, originChain, req.Asset.Name) {
return nil, fmt.Errorf("asset %s is not registered on the origin chain %s", originChain.NativeAsset, originChain.Name)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why dont' we need to check if the asset is registered for external tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cuz when deploying an external token, this is the process of having it registered.

x/evm/types/msg_create_deploy_token.go
return &CreateDeployTokenRequest{
Sender: sender,
Chain: utils.NormalizeString(chain),
Asset: asset,
TokenDetails: tokenDetails,
MinAmount: minAmount,
Address: address,
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont' we need to update Validate with a check for this new field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Address is of type byte[20] and therefore there's nothing to be validated.

@fish-sammy fish-sammy force-pushed the feat/external-erc20-support branch 5 times, most recently from a00aedd to 8a182a6 Compare January 19, 2022 20:19
@fish-sammy fish-sammy marked this pull request as ready for review January 19, 2022 20:19
@fish-sammy fish-sammy force-pushed the feat/external-erc20-support branch 15 times, most recently from 24976a7 to 0f5082c Compare January 24, 2022 20:07
@fish-sammy fish-sammy force-pushed the feat/external-erc20-support branch 6 times, most recently from 550fe1d to 06ecce4 Compare January 26, 2022 17:12
jcs47
jcs47 reviewed Jan 26, 2022
View reviewed changes
x/evm/legacy/params.go
@@ -0,0 +1,163 @@
package legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

this is basically a copy of params.go before adding the new field, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct!

jcs47
jcs47 reviewed Jan 26, 2022
View reviewed changes
x/evm/types/expected_keepers.go
GetTokenByteCodes(ctx sdk.Context) ([]byte, bool)
GetGatewayByteCode(ctx sdk.Context) ([]byte, bool)
GetBurnerByteCode(ctx sdk.Context) ([]byte, bool)
GetTokenByteCode(ctx sdk.Context) ([]byte, bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not have one for the absorver too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it like

axelar-core/x/evm/keeper/chainKeeper.go

Line 273 in c10d473

bytecode := k.GetParams(ctx).Absorber

jcs47
jcs47 approved these changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@jcs47 jcs47 left a comment

Choose a reason for hiding this comment

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

LGTM, but better wait for Chis and/or Haiyi to also check this out

cgorenflo
cgorenflo approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@cgorenflo cgorenflo left a comment

Choose a reason for hiding this comment

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

the external erc20 deployment is still under the controller keys control, right

x/evm/module.go
@@ -156,6 +156,7 @@ func (am AppModule) LegacyQuerierHandler(*codec.LegacyAmino) sdk.Querier {
// module-specific GRPC queries.
func (am AppModule) RegisterServices(cfg module.Configurator) {
types.RegisterQueryServiceServer(cfg.QueryServer(), am.keeper)
cfg.RegisterMigration(types.ModuleName, 1, keeper.GetMigrationHandler(am.keeper, am.nexus))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this registered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's where cosmos-hub did it so I just assumed that this is the best place for it.

@fish-sammy
Copy link
Contributor Author

the external erc20 deployment is still under the controller keys control, right

@cgorenflo Yes since it's the same type of tx

@fish-sammy fish-sammy merged commit c8c964f into main Jan 27, 2022
@fish-sammy fish-sammy deleted the feat/external-erc20-support branch January 27, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haiyizxx haiyizxx haiyizxx approved these changes

@jcs47 jcs47 jcs47 approved these changes

@cgorenflo cgorenflo cgorenflo approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

external erc-20
4 participants
@fish-sammy @haiyizxx @jcs47 @cgorenflo