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: permissioned creation of token metadata #327

Merged
merged 31 commits into from
Mar 21, 2024
Merged

feat: permissioned creation of token metadata #327

merged 31 commits into from
Mar 21, 2024

Conversation

hoangdv2429
Copy link
Contributor

PR Standards

Close #312

Opening a pull request should be able to meet the following requirements

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@hoangdv2429 hoangdv2429 requested a review from a team as a code owner March 17, 2024 18:23
@omritoptix omritoptix changed the title Feat/permissioned_tokenmetadata feat: permissioned creation of token metadata Mar 17, 2024
@mtsitrin
Copy link
Collaborator

we need to have hooks available as we did on the hub
it will be connected to x/erc20 module on the rollapp-evm

zale144
zale144 previously approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

lgtm, just a couple of tiny points

x/denommetadata/keeper/msg_server.go Show resolved Hide resolved
testutil/keepers/keepers.go Outdated Show resolved Hide resolved
omritoptix
omritoptix previously approved these changes Mar 19, 2024
View reviewed changes
omritoptix
omritoptix previously approved these changes Mar 19, 2024
View reviewed changes
@hoangdv2429
Copy link
Contributor Author

I need to refactor with App that merged in #321

mtsitrin
mtsitrin reviewed Mar 20, 2024
View reviewed changes
x/denommetadata/types/keys.go Outdated Show resolved Hide resolved
testutil/app/app.go Show resolved Hide resolved
danwt
danwt previously approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

looks good
left some optional nits

x/denommetadata/keeper/keeper.go Outdated Show resolved Hide resolved
x/denommetadata/keeper/keeper.go Outdated Show resolved Hide resolved
x/denommetadata/keeper/keeper.go Outdated Show resolved Hide resolved
x/denommetadata/types/hooks.go Outdated Show resolved Hide resolved
x/denommetadata/types/keys.go Outdated Show resolved Hide resolved
x/denommetadata/types/keys.go Outdated Show resolved Hide resolved
x/denommetadata/types/msg.go Show resolved Hide resolved
x/denommetadata/types/params.go Outdated Show resolved Hide resolved
x/denommetadata/types/params.go Show resolved Hide resolved
x/denommetadata/types/params.go Outdated Show resolved Hide resolved
@danwt danwt self-requested a review March 20, 2024 09:54
mtsitrin
mtsitrin requested changes Mar 20, 2024
View reviewed changes
testutil/app/app.go Show resolved Hide resolved
mtsitrin
mtsitrin reviewed Mar 20, 2024
View reviewed changes
x/denommetadata/module.go Outdated Show resolved Hide resolved
mtsitrin
mtsitrin previously approved these changes Mar 20, 2024
View reviewed changes
@omritoptix
Copy link
Contributor

@hoangdv2429 build fails

@omritoptix omritoptix merged commit ffe8e06 into dymensionxyz:main Mar 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtsitrin mtsitrin mtsitrin approved these changes

@omritoptix omritoptix omritoptix approved these changes

@zale144 zale144 zale144 left review comments

@danwt danwt Awaiting requested review from danwt

Assignees

@danwt danwt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a way to register denom metadata using a permissioned address
6 participants
@hoangdv2429 @mtsitrin @omritoptix @zale144 @danwt @trinitys7