-
Notifications
You must be signed in to change notification settings - Fork 367
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
Issuance module #599
Issuance module #599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, very thorough. The owner should have the ability to unblock addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks solid. I tried to break stuff but couldn't find anything
Only request is tidying up module.go
- the tx and query functions are empty.
some nits:
- an asset owner could mint & send coins to module accounts, which might cause problems elsewhere. If the owner is minting coins automatically, they won't be checking to see if a user has malliciously submitted a mod account address as the destination address.
- When blocked addr coins are seized, it could confuse the accountant managing the owner address. Maybe emitt an event when coins are seized to help track where they came from?
- some msg ValidateBasic tests would be nice
I had a think about ways to pause asset transactions, but the best I came up with was to seize everyone's coins when pause is activated, then pay them back when unpaused. But that's a bit extreme and doesn't handle coins being in other places like cdps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about moving the Assets out from the Params 👍
|
||
// Params governance parameters for the issuance module | ||
type Params struct { | ||
Assets Assets `json:"assets" yaml:"assets"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so I think Assets should be governance params. They should be set to the issuance module store and defined on genesis. If a new asset needs to be added/removed you can create a custom gov proposal type for both operations and create a custom handler to add/delete assets from the store.
This way you have fine-grained control over the validation of the assets, eg validate that users cannot add and delete assets on the same param change proposal. Also, param change proposals don't allow you to validate the new value with the previously stored ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assets already are governance params because they are part of the Params
for the module, so we can use param-change
proposals to add/remove/edit assets. I think this is sufficient for governance for now, can expand if necessary as scope becomes better defined.
Created an issue on cosmos-sdk for enabling/disabling coin transfers of individual coins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
x/issuance/keeper/issuance.go
Outdated
@@ -98,6 +100,33 @@ func (k Keeper) BlockAddress(ctx sdk.Context, denom string, owner, blockedAddres | |||
return nil | |||
} | |||
|
|||
// UnblockAddress adds an address to the blocked list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// UnblockAddress adds an address to the blocked list | |
// UnblockAddress removes an address from the blocked list |
x/issuance/types/msg.go
Outdated
Address sdk.AccAddress `json:"address" yaml:"address"` | ||
} | ||
|
||
// NewMsgUnblockAddress returns a new MsgIssueTokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NewMsgUnblockAddress returns a new MsgIssueTokens | |
// NewMsgUnblockAddress returns a new MsgUnblockAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 👍
|
||
// String implements fmt.Stringer | ||
func (msg MsgUnblockAddress) String() string { | ||
return fmt.Sprintf(`Unblock Address: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit we should use yaml
Note: The v0.3 -> v0.8 migration tests will need to be updated with a custom app type for
v0.8
, then that type should be used in the migration tests. Out of scope for this PR.