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: NTRN-332. Add tokenfactory module #131

Merged
merged 9 commits into from
Apr 3, 2023

Conversation

albertandrejev
Copy link
Member

@albertandrejev albertandrejev commented Jan 24, 2023

Osmosis has the token factory module that allows anyone to create new native tokens. We want to have it in Neutron, so we need to see if it’s possible to easily include it to our codebase: osmosis/x/tokenfactory at main · osmosis-labs/osmosis

Main reason why we decided to copypaste code because we also want to integrate ibc hooks module as well and wanted to have everything under our control.

Dependent:
neutron-org/neutron-integration-tests#75
neutron-org/neutron-docs#52

Depends on:
neutron-org/neutron-dev-contracts#8

Task: NTRN-332
Test run

@albertandrejev albertandrejev changed the base branch from main to neutron_audit_informal_17_01_2023_fixes January 24, 2023 10:19
@albertandrejev albertandrejev changed the base branch from neutron_audit_informal_17_01_2023_fixes to neutron_audit_informal_17_01_2023 January 24, 2023 10:20
@albertandrejev albertandrejev changed the title feat: NTRN-332. Add treasury module feat: NTRN-332. Add tokenfactory module Jan 24, 2023
go.mod Outdated
@@ -137,5 +135,6 @@ replace (
github.com/cosmos/admin-module => github.com/Ethernal-Tech/admin-module v0.0.0-20221102105340-e693f4d379c3
github.com/cosmos/ibc-go/v3 => github.com/informalsystems/ibc-go/v3 v3.0.0-beta1.0.20220816140824-aba9c2f2b943
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
github.com/osmosis-labs/osmosis/v10 v10.2.0 => github.com/neutron-org/osmosis/v10 v10.2.0-native-sdk-4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need a separate branch in our osmosis fork (like we did with wasmd fork) where we will push necessary changes for us.
Also it's easier to see the diff

Copy link
Member Author

Choose a reason for hiding this comment

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

done

zavgorodnii
zavgorodnii previously approved these changes Feb 1, 2023
@albertandrejev albertandrejev marked this pull request as draft February 7, 2023 09:09
@albertandrejev albertandrejev force-pushed the feat/NTRN-332-add-token-factory branch from 9494728 to 4c674b1 Compare March 2, 2023 13:27
@zavgorodnii zavgorodnii marked this pull request as ready for review March 7, 2023 13:18
@foxpy foxpy requested review from foxpy and zavgorodnii March 7, 2023 13:19
Copy link
Collaborator

@pr0n00gler pr0n00gler left a comment

Choose a reason for hiding this comment

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

We should leave an info that this module is a copy paste from Osmosis.
And maybe some License file

@larry0x
Copy link

larry0x commented Mar 8, 2023

ICYMI, there is a maintained standalone repo for token-factory, so you can perhaps avoid copy-pasting the entire module codebase into this repo: https://github.com/CosmWasm/token-factory

@larry0x
Copy link

larry0x commented Mar 8, 2023

Also, I don't see wasm bindings for token-factory. The bindings are needed for contracts to actually use the module. See Osmosis' bindings here: https://github.com/osmosis-labs/osmosis/blob/main/wasmbinding/bindings/msg.go#L6-L17

@albertandrejev
Copy link
Member Author

ICYMI, there is a maintained standalone repo for token-factory, so you can perhaps avoid copy-pasting the entire module codebase into this repo: https://github.com/CosmWasm/token-factory

Yes we saw it but thank you anyway. We decided to copypaste code because we also want to integrate ibc hooks module as well and wanted to have everything under our control.

@albertandrejev
Copy link
Member Author

Also, I don't see wasm bindings for token-factory. The bindings are needed for contracts to actually use the module. See Osmosis' bindings here: https://github.com/osmosis-labs/osmosis/blob/main/wasmbinding/bindings/msg.go#L6-L17

Thanks for pointing this out. I will implement it as well!

@pacmanifold
Copy link

@albertandrejev there has been some recent development on the token factory in the osmosis repo to introduce side effects.

osmosis-labs/osmosis#4382
osmosis-labs/osmosis#4417
osmosis-labs/osmosis#4590

Any chance these features could be included in the integration of token factory into neutron. It would be extremely useful 🙏

@albertandrejev
Copy link
Member Author

@albertandrejev there has been some recent development on the token factory in the osmosis repo to introduce side effects.

osmosis-labs/osmosis#4382 osmosis-labs/osmosis#4417 osmosis-labs/osmosis#4590

Any chance these features could be included in the integration of token factory into neutron. It would be extremely useful pray

Thank you for your message but there is difficulties to implement some of these features:

  1. Tokenfactory: Add Before send hooks osmosis-labs/osmosis#4382 requires modified version of Cosmos SDK (Hooks support for the Bank module)
  2. [TokenFactory] Add ForceTranfer, BurnFrom and MintTo osmosis-labs/osmosis#4417 we need to discuss it
  3. [TokenFactory] Bank Hooks Integration osmosis-labs/osmosis#4590 same issue with modified version of Cosmos SDK

@albertandrejev albertandrejev force-pushed the feat/NTRN-332-add-token-factory branch from 6703089 to 5ea153d Compare March 20, 2023 19:24
testutil/test_helpers.go Outdated Show resolved Hide resolved
wasmbinding/bindings/query.go Outdated Show resolved Hide resolved
wasmbinding/custom_querier.go Outdated Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
wasmbinding/message_plugin.go Outdated Show resolved Hide resolved
wasmbinding/queries.go Outdated Show resolved Hide resolved
@albertandrejev albertandrejev requested a review from foxpy March 21, 2023 14:17
x/tokenfactory/README.md Show resolved Hide resolved
x/tokenfactory/README.md Show resolved Hide resolved
x/tokenfactory/README.md Show resolved Hide resolved
x/tokenfactory/README.md Show resolved Hide resolved
x/tokenfactory/README.md Show resolved Hide resolved
x/tokenfactory/client/cli/query.go Outdated Show resolved Hide resolved
x/tokenfactory/client/cli/tx.go Show resolved Hide resolved
x/tokenfactory/client/cli/tx.go Show resolved Hide resolved
x/tokenfactory/keeper/admins_test.go Show resolved Hide resolved
x/tokenfactory/keeper/admins_test.go Show resolved Hide resolved
x/tokenfactory/keeper/admins_test.go Outdated Show resolved Hide resolved
x/tokenfactory/keeper/admins_test.go Show resolved Hide resolved
x/tokenfactory/keeper/admins_test.go Show resolved Hide resolved
x/tokenfactory/keeper/keeper.go Show resolved Hide resolved
x/tokenfactory/keeper/keeper_test.go Show resolved Hide resolved
x/tokenfactory/keeper/keeper_test.go Show resolved Hide resolved
x/tokenfactory/types/msgs.go Show resolved Hide resolved
x/tokenfactory/types/msgs.go Show resolved Hide resolved
@foxpy foxpy self-requested a review March 22, 2023 13:53
@albertandrejev albertandrejev changed the base branch from neutron_audit_informal_17_01_2023 to main March 22, 2023 14:00
@albertandrejev albertandrejev dismissed zavgorodnii’s stale review March 22, 2023 14:00

The base branch was changed.

foxpy
foxpy previously approved these changes Mar 23, 2023
@albertandrejev albertandrejev force-pushed the feat/NTRN-332-add-token-factory branch from c797f61 to d73669a Compare March 27, 2023 20:39
@albertandrejev albertandrejev requested a review from foxpy March 28, 2023 09:06
Comment on lines +35 to +44
CreateDenom *CreateDenom `json:"create_denom,omitempty"`
/// Contracts can change the admin of a denom that they are the admin of.
ChangeAdmin *ChangeAdmin `json:"change_admin,omitempty"`
/// Contracts can mint native tokens for an existing factory denom
/// that they are the admin of.
MintTokens *MintTokens `json:"mint_tokens,omitempty"`
/// Contracts can burn native tokens for an existing factory denom
/// that they are the admin of.
/// Currently, the burn from address must be the admin contract.
BurnTokens *BurnTokens `json:"burn_tokens,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are bindings on Neutron side but I don't necessary changes in our cosmwasm neurton-sdk. Am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

missed this point, thanks!


bz, err := json.Marshal(res)
if err != nil {
return nil, fmt.Errorf("failed to JSON marshal DenomAdminResponse response: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You use sdkerrors.Wrap everywhere above, but here you use fmt.Errorf. Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

case contractQuery.DenomAdmin != nil:
res, err := qp.GetDenomAdmin(ctx, contractQuery.DenomAdmin.Subdenom)
if err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap an error with some meaningful description

_, err = msgServer.Mint(sdk.WrapSDKContext(ctx), sdkMsg)
if err != nil {
return sdkerrors.Wrap(err, "minting coins from message")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not critical, but we usually leave \n after error handling

pr0n00gler
pr0n00gler previously approved these changes Mar 31, 2023
foxpy
foxpy previously approved these changes Apr 3, 2023
@pr0n00gler pr0n00gler dismissed stale reviews from foxpy and themself via 54cde79 April 3, 2023 14:21
@pr0n00gler pr0n00gler merged commit b112fe5 into main Apr 3, 2023
@pr0n00gler pr0n00gler deleted the feat/NTRN-332-add-token-factory branch January 11, 2024 14:22
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.

6 participants