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

chore: x/tokenfactory audit #1962

Merged
merged 8 commits into from
Jul 5, 2022
Merged

chore: x/tokenfactory audit #1962

merged 8 commits into from
Jul 5, 2022

Conversation

alexanderbez
Copy link
Contributor

Closes: #1914

What is the purpose of the change

Auditing the x/tokenfactory module

Brief Changelog

  • Revising proto comments
  • Revising keeper and types godocs
  • Revising tests

@alexanderbez
Copy link
Contributor Author

Not sure why, but after modifying the proto schemas and running make proto-gen, I get no updated compiled files...

@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/tokenfactory labels Jul 5, 2022
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 5, 2022
@alexanderbez alexanderbez marked this pull request as ready for review July 5, 2022 16:37
@alexanderbez alexanderbez requested a review from a team July 5, 2022 16:37
@p0mvn
Copy link
Member

p0mvn commented Jul 5, 2022

Not sure why, but after modifying the proto schemas and running make proto-gen, I get no updated compiled files...

I tried running make proto-all on my side and saw some updates to pb.go files.

Maybe try make proto-all instead of proto-gen? If it doesn't generate anything in your local environment, please let me know. I can try to make a commit from my side to see if that fixes it

proto/osmosis/tokenfactory/v1beta1/tx.proto Show resolved Hide resolved
x/tokenfactory/README.md Outdated Show resolved Hide resolved
allows changing the master admin account, or even setting it to
`""`, meaning no account has admin privileges of the asset.

- Change the admin In the future, more admin capabilities may be added. Admins
Copy link
Member

Choose a reason for hiding this comment

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

Asking to learn - what are the current admin capabilities? They are not implemented yet, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, current admin caps are just who created the token and that gives them the ability to perform certain actions. It does not extend beyond that AFAIK.

cc @sunnya97

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current admin capabilities are only minting and burning

- Burning of a specific denom is only allowed for the creator of the denom registered during `CreateDenom`
``` {.go}

Burning of a specific denom is only allowed for the creator of the denom
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 this text might be about Burn, not ChangeAdmin

x/tokenfactory/module.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@p0mvn I tried both commands and I get no diff. I'm not sure but maybe it's because I'm on an m1 chip? Regardless, I have zero issues compiling proto in the SDK repo...

@p0mvn
Copy link
Member

p0mvn commented Jul 5, 2022

@p0mvn I tried both commands and I get no diff. I'm not sure but maybe it's because I'm on an m1 chip? Regardless, I have zero issues compiling proto in the SDK repo...

That's weird. Probably, a machine-specific issue. I made #1971 to your PR to see if that fixes CI. This is what my Linux machine produces from your branch

@p0mvn p0mvn mentioned this pull request Jul 5, 2022
@ValarDragon
Copy link
Member

Did you find nothing odd in the logic / tests / events of token factory?

@alexanderbez
Copy link
Contributor Author

Did you find nothing odd in the logic / tests / events of token factory?

I think test coverage could improve, but what is there there now looks sufficient.

@ValarDragon ValarDragon merged commit 133e139 into main Jul 5, 2022
@ValarDragon ValarDragon deleted the bez/1914-tokenfactory-audit branch July 5, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation C:x/tokenfactory
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

x/tokenfactory - internal audit
4 participants