feat: lock token transfer and parameter module, take 2 #3860
Conversation
|
I'm done with commits unless there's anything to fix. |
| if m.Frames[len(m.Frames)-1].LastPackage.PkgPath != "sys/params" { | ||
| panic("should not happen") | ||
| } | ||
| if m.Frames[len(m.Frames)-2].LastPackage.PkgPath != "gno.land/r/sys/params" { |
There was a problem hiding this comment.
| if m.Frames[len(m.Frames)-2].LastPackage.PkgPath != "gno.land/r/sys/params" { | |
| if m.Frames[len(m.Frames)-2].LastPackage.PkgPath != m.ChainDomain()+"/r/sys/params" { |
not urgent.
moul
force-pushed
the
feat_lock_transfer
branch
2 times, most recently
from
0fc4ce2 to
18e6576
Compare
|
merged master on the PR and applied various fixes to make the CI pass (642a2cf) |
|
Merging now. I suggest people to make post-merge reviews. cc @thehowl @zivkovicmilos @piux2 @aeddi |
There was a problem hiding this comment.
I don't have any excessive comments that we didn't cover in the original params / token lock PR.
Looks solid 💯
Left a few questions, mostly for future implementation choices.
I couldn't find anything that immediately jumps out. I think I've cleaned up some panic logic and swapped it for error propagation in an old draft PR I have, that's been blocked for a while.
| @@ -18,8 +20,82 @@ | |||
| ErrBalanceEmptyAmount = errors.New("balance amount is empty") | |||
| ) | |||
|
|
|||
| const ( | |||
| // XXX rename these to flagXyz. | |||
| // flagUnrestricted allows flagUnrestricted transfers. | ||
| flagUnrestricted BitSet = 1 << iota | ||
|
|
||
| // TODO: flagValidatorAccount marks an account as validator. |
There was a problem hiding this comment.
Interesting, I have questions :)
What is the use case for marking an account as belonging to a validator?
I assume it's probably for some kind of distribution mechanism.
As for realm accounts, I assume it's for account sessions. I still have to think about this, but I'm not sure if this is something that we should do protocol-level instead of marking accounts as "sessionable" in the params.
| @@ -423,6 +423,7 @@ func isAllZero(key [64]byte) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // XXX why is this needed? | |||
|
|
||
| // XXX: ListKeys? | ||
| type ParamfulKeeper interface { | ||
| WillSetParam(ctx sdk.Context, key string, value interface{}) |
There was a problem hiding this comment.
Just curious: why do we need a context for setting a kv pair?
I don't anticipate this is an action that is not instantaneous
|
|
||
| func (bank BankKeeper) WillSetParam(ctx sdk.Context, key string, value interface{}) { | ||
| switch key { | ||
| case "p:restricted_denoms": // XXX test |
There was a problem hiding this comment.
Nit: outdated comment, test exists
| } | ||
|
|
||
| func (p Param) ValidateBasic() error { | ||
| // XXX: validate type and value |
| StoreKey = ModuleName | ||
| // ValueStorePrevfix is "/pv/" for param value. | ||
| ValueStoreKeyPrefix = "/pv/" | ||
| // StoreKey = ModuleName |
|
Please feel free to resolve my comments and merge -- there is nothing blocking that I left |
This is a continuation of #3176.
It breaks the params struct into fields and stores them in ::. All parameters are of the form : or :<realm//*>:. A colon is used rather than a period/dot because it's ambiguous with realm domains.
std.SetParam is realm-local only, limited and safe. sys/params.SetSysParam is arbitrary and global. Need to make sure it's only importable by r/sys/params, but I think we said we'd do this with "internal" or something. Maybe for a followup PR.
The ParamsKeeper is divided into prefix spaces,
pk.WithModule("bank")for example, and that is passed into the BankKeeper, that way the BankKeeper only has access to the bank:* param space. "language capabilities" security. But the VMKeeper needs to the root ParamsKeeper because it needs to be able to modify anything via r/sys/params.I got rid of the .type suffix of param keys. It's less safe right now but we can add type safety to this later. You can and must use the type suffix from toml for certain types though; "strings" which represent []strings must be suffixed by .strings, because toml only gives []interface{}{}.