feat: lock token transfer and parameter module #3176
Conversation
github-actions
bot
added
🧾 package/realm
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Thank you for pushing this out 🙏
I think the direction is good, but I'd love to discuss more about some implementation details. I'm mostly worried about us having a temporary implementation detail as a permanent state of a core object (check comments).
Pinging @moul to give a review as well.
Please check the CI 🙏
| stdout '(0 uint64)' | ||
|
|
||
|
|
||
| ## vote unlock proposal with unrestricted account test1 |
There was a problem hiding this comment.
Which accounts are going to be unrestricted in the initial version of the chain?
There was a problem hiding this comment.
For now, all GovDAO accounts that need to vote must be unrestricted from token transfer locking, as voting on a proposal requires sending fees to the contract.
gno.land/pkg/gnoland/app_test.go
Outdated
| {key: "foo", kind: "bool", value: true}, | ||
| {key: "foo", kind: "bytes", value: []byte{0x48, 0x69, 0x21}}, | ||
| }, | ||
| /* |
There was a problem hiding this comment.
This is commented out since it conflicts with the alternative approach for the Params module in this PR. I believe we should avoid setting arbitrary parameter values in the genesis file without going through module validation.
We need to discuss it and agree on the approach.
tm2/pkg/sdk/auth/genesis.go
Outdated
| // Otherwise, we cannot verify the unrestricted address in the genesis state. | ||
|
|
||
| for _, addr := range data.Params.UnrestrictedAddrs { | ||
| acc := ak.GetAccount(ctx, addr) |
There was a problem hiding this comment.
There is tight coupling here. The account keeper must have a previously initialized account in order for us to modify it in another func
Can't we keep the store of unrestricted accounts somewhere, instead of keeping the Unrestricted information on the account itself?
My biggest concern is that we're coupling temporary logic (account balance transfer locks) into a structure that will be encoded and saved permanently to disk. Every time we use the account, even in 10 years, we will have to keep track of its "restricted state".
This is why I'm suggesting we go with an approach that isn't so coupled
There was a problem hiding this comment.
There are two reasons for implementing it this way:
-
There was a previous requirement to track and unlock token transfers on an individual account basis, not just for whitelisted accounts. That is the reason the restricted state is on the account level. We can revisit the requirement, otherwise, we will need to track the restricted state for a long time.
-
The whitelisted unrestricted accounts need to be validated to ensure they exist when we load the genesis state.
There was a problem hiding this comment.
into a structure that will be encoded and saved permanently to disk.
It is something that new chains will probably need for US regulatory purposes so it might be something we want to support long term.
Also, we can use a bitmask (uint64) instead of a bool, to save space, and we can have more flags.
- There was a previous requirement to track and unlock token transfers on an individual account basis
Is there more info on this?
- The whitelisted unrestricted accounts need to be validated to ensure they exist when we load the genesis state.
doesn't seem like a reason that necessitates this solution?
still thinking...
tm2/pkg/sdk/auth/params.go
Outdated
| ok, err := ak.paramk.GetParams(ctx, ModuleName, "p", params) | ||
|
|
||
| if !ok { | ||
| panic("params key " + ModuleName + " does not exist") |
There was a problem hiding this comment.
The module parameter keys are predefined and store in the Param module. If we try to access a predefined key that does not exist, the program should panic.
tm2/pkg/sdk/auth/params.go
Outdated
| TxSizeCostPerByte int64 `json:"tx_size_cost_per_byte" yaml:"tx_size_cost_per_byte"` | ||
| SigVerifyCostED25519 int64 `json:"sig_verify_cost_ed25519" yaml:"sig_verify_cost_ed25519"` | ||
| SigVerifyCostSecp256k1 int64 `json:"sig_verify_cost_secp256k1" yaml:"sig_verify_cost_secp256k1"` | ||
| UnrestrictedAddrs []crypto.Address `json:"unrestricted_addrs" yaml:"unrestricted_addrs"` |
There was a problem hiding this comment.
Just a note we don't initialize this new field in DefaultParams
There was a problem hiding this comment.
By default, there are no unrestricted accounts since token transfer locking is off. Unrestricted accounts should be added to the genesis file when token locking is set to true. These accounts can be added manually or through a separate genesis command later in the production deployment.
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
I was asked by Manfred to provide some feedback on the two different approaches. I don't have strong opinions one way or the other, here's what I consider:
If I was forced to choose, the "generic" approach seems to me simple to understand and to build on top of, and like I would shoot myself less frequently in the foot. But I'm not heavily swinging either way. |
|
@thehowl @moul The issue is not limited to key name and type validation. Allowing the creation and update of arbitrary chain parameters is unsafe and could lead to undetectable mistakes, potentially leaving the chain vulnerable to exploitation later. It's like using a map when we should be using a struct to pass parameters to the critical sections of a program. Here are some specifics:
In practice, proposals like this are difficult to verify in terms of their behavior and consequences because the behavior is implemented in the code itself.
Configurations can easily become out of sync with the code implementation without being noticed. This makes it challenging to track how these settings are linked in the code and how they impact chain behavior. This issue is especially problematic when we need to support backward-compatible features.
This constant does not match the genesis configuration, meaning changes in the genesis will never update the value of chainDomainParamPath as intended. gno/gno.land/genesis/genesis_params.toml Line 10 in 3fd5571 gno/gno.land/pkg/sdk/vm/params.go Line 7 in 3fd5571 It could get worse. Imagine if this property were created by GovDao instead of the genesis. All these small mistakes would be extremely difficult to detect. The old key could remain in the chain even after creating a new key |
| @@ -1,132 +0,0 @@ | |||
| package main | |||
There was a problem hiding this comment.
What do you think about introducing a filetest for the locking, since we dropped this string param one?
There was a problem hiding this comment.
The non-typed method of using r/sys/params module is and still should be supported, so this test shouldn't be deleted.
There was a problem hiding this comment.
@jaekwon
It was not deleted because of a non-typed method. Instead, it was removed because the original params.gno and params_test.gno allowed adding arbitrary values as key-value pairs as chain parameters from GovDAO. This could lead to approving proposals without fully understanding their consequences since the key-value pairs may not be easily traceable in the code. Additionally, there is a risk of approving a misspelled key name that does not correspond to any logic in the code, potentially causing false-positive effects.
The example of this type of bug already been found as I mentioned here
#3176 (comment)
I agree that the current suggested implementation we reached consensus on reduces the risk of modifying the parameters of the modules.
It still requires all of us to understand and follow the convention: all chain parameters must be predefined in the module. However, this does not prevent us from implementing something outside the parameters convention in our logic that takes key-value pairs directly from GovDAO, which may mismatch within proposals. We should stay extra alert about this.
|
I'll try to sum up my primary concerns with this PR, and we just need to agree if we're okay with this at this point in the chain's lifecycle: Things that keep me worried about the implementation:
What I like about the PR:
In terms of which approach to go with, maybe we can opt for a fusion of the two ideas?
I'm more inclined to opt for the field approach Manfred initially advocated for, because it seems more durable and future-proof. We already support most field types, so I can't foresee an issue when we need to change values or update structs (completely revamp what "params" are). We also don't encounter the |
|
This PR supports both individual field parameters and struct types. The most important gap we aim to resolve between this PR and the existing params implementation is the following: a) The current implementation treats the Param module as a native store, allowing arbitrary chain parameter key-value pairs to be added from both genesis and GovDao. b) This PR predefines chain parameters in the code, allowing only their values to be modified from genesis and GovDao. |
gno.land/pkg/sdk/vm/builtins.go
Outdated
| func (prm *SDKParams) SetBool(key string, value bool) { | ||
| prm.assertRealmAccess() | ||
| realmParamKey := fmt.Sprintf("%s.%s", prm.curRealmPath, lockSendKey) | ||
| if key == realmParamKey { |
There was a problem hiding this comment.
this can't be the right design.
-
curRealmPath itself is weird; it's a variable field of SDKParams, presumably it can change values, even though it's only supposed to be r/sys/params today, it would be easy to suggest a change that removes this restriction. then, this
if key == realmParamKeycondition could succeed where it should not, because another curRealmPath tried to use lockSendKey. -
we're going to end up with all these custom prm.vmk.xxx.Yyy() injections in this file, but they would be grouped by type. I'm not sure exactly how, but really they should be grouped by the realm part of the key, if anything. And adding onto the point 1 above, the realm path should be extracted from the key, and compared against a const, not depend on curRealmPath.
thinking...
There was a problem hiding this comment.
Suggested modification:
func (prm *SDKParams) SetBool(key string, value bool) {
prm.assertRealmAccess(key) // uses the key to assert
kpr := prm.keeperMapper.GetKeeper(key) // keeperMapper defined in https://github.com/gnolang/gno/pull/3176/files#r1908698885
if kpr != nil {
kpr.WillSetParam(key, value) // could be WillSetBool() but WillSetParam() seems simpler.
}
prm.vmk.prmk.SetBool(prm.ctx, key, value)
}Better yet I think is to just merge keeperMapper into SDKParams, and make SDKParams something that takes the context explicitly (like a keeper), and doesn't go through vmk to access needed keepers. See comments on SDKParams for more info.
func (prm *SDKParams) SetBool(ctx sdk.Context, key string, value bool) {
prm.assertRealmAccess(key) // uses the key to assert
kpr := prm.GetRegisteredKeeper(key)
if kpr != nil {
kpr.WillSetParam(ctx, key, value) // could be WillSetBool() but WillSetParam() seems simpler.
}
prm.prmk.SetBool(ctx, key, value)
}There was a problem hiding this comment.
Agree, see #3176 (comment)
gno.land/pkg/sdk/vm/params.go
Outdated
| } | ||
|
|
||
| func (vm *VMKeeper) SetParams(ctx sdk.Context, params Params) error { | ||
| if params.Equals(Params{}) { |
There was a problem hiding this comment.
I don't see why we'd want this block.
gno.land/pkg/gnoland/app.go
Outdated
| km := params.NewPrefixKeyMapper() | ||
| km.RegisterPrefix(auth.ParamsPrefixKey) | ||
| km.RegisterPrefix(bank.ParamsPrefixKey) | ||
| km.RegisterPrefix(vm.ParamsPrefixKey) |
There was a problem hiding this comment.
How about instead of PrefixKeyMapper, we call it KeeperMapper, and we register not just the prefix key, but we register the keeper itself?
e.g. auth, bank, vm could implement the following interface
type ParamfulKeeper interface {
GetParamsKey() string
}and km.RegisterKeepers(auth, bank, vm).
This way, we can actually do a lookup in Params.SetBool (for example) to look up the keeper first by prefix.
See specific suggestion in https://github.com/gnolang/gno/pull/3176/files#r1908697754 which actually suggests removing PrefixKeyMapper/KeeperMapper entirely and just baking it into SDKParams directly.
| @@ -62,19 +65,59 @@ func (bnk *SDKBanker) RemoveCoin(b32addr crypto.Bech32Address, denom string, amo | |||
| type SDKParams struct { | |||
| vmk *VMKeeper | |||
| ctx sdk.Context | |||
| // The curRealmPath is used to track the current realm accessing the SDKParams from the VM. | |||
| // It serves as a safeguard to control access from the VM. | |||
| curRealmPath string | |||
| } | |||
There was a problem hiding this comment.
I think ctx should be removed, and curRealmPath removed for sure, but also maybe vmk removed too, and replaced with the following:
type SDKParams struct {
pmk *ParamsKeeper
kprs map[prefix]ParamfulKeeper
}And the methods like SetBool(key, string, value bool) take ctx explicitly, much like a keeper itself.
func (prm *SDKParams) SetBool(ctx sdk.Context, key string, value bool) ...By passing in ctx in the methods, there is no need to create new SDKParams instances for each context.
And instead of accessing keepers indirectly via prm.vmk.bank.AddRestrictedDenoms, the bank keeper can just be registered as a ParamfulKeeper in prm directly. One benefit is that keepers that aren't even relevant to the VMKeeper can be registered this way. And besides, if you need to access a private field keeper of bank (e.g. prm.vmk.bank.other.XYZ()), you can't do it due to Go's security model, and that's the whole point of keepers anyways.
There was a problem hiding this comment.
I kept SDKParams as a wrapper of ParamKeeper, as suggested. However, we cannot pass sdk.Context directly to SetBool and use it as a normal keeper without significant refactoring and changes. This is because func (prm *SDKParams) SetBool() is called from the bound native methods through generated code. In the native function call flow, sdk.Context does not exist in the call flow.
It seems the existing approach is the only way to get sdk.Context in SDKParams. This approach keeps sdk.Context as a state of SDKParams when it is instantiated in the new Machine, and then uses the context when the native call is triggered.
I also moved kprs map[prefix]ParamfulKeeper to ParamsKeeper so that it can register the auth, bank, and VM keepers when the app is initialized.
Please let me know, if this approach works.
I don't have an opinion on this yet, except to say that it belongs in GnoAccount (and we can in the future move them to BaseAccount if other projects want to use it); and that bitflags kind of mitigate the problem. See comment in code.
This is my primary concern; adding a new param to a keeper would require migration code, unless amino supports appending fields (without changing the order of prior fields). But maybe that is the answer. Otherwise I don't see how per-module param updates can be maintained cleanly, except what Milos mentioned; basically breaking down the per-module param fields into individual values, and storing them separately, yet still having the module maintain the list of parameters, and somehow also managing upgrades/additions/deprecations of per-module param keys.
I think this can be addressed by the ParamfulKeeper and keeperMapper baked directly into SDKParams directly. See prior comments on files for more info, namely https://github.com/gnolang/gno/pull/3176/files#r1908697754, https://github.com/gnolang/gno/pull/3176/files#r1908698885, and https://github.com/gnolang/gno/pull/3176/files#r1908743583. And check the comments on deleted files; even if we support this per-module typed param system, we still want to support the previous system, so the original tests should not be deleted. For one, if we're going to base SDKParam on the ParamKeeper, it makes sense for ParamKeeper to have good tests independet of SDKParam. But also, I can still imagine keepers and non-keepers needing params that are more free-form; for example, parameters for a specific gno realm, such as per-account information, stored in the ParamKeeper with a key like /vm/realmparams/gno.land/r/something..Username = "Bob". The reason for using such a system might be that this allows for fast per-account username lookup without needing to access the Gno value tree or VM, but rather just an AVL tree lookup (i.e. optimization).
I think this is worth considering too before merging. |
Done
There is only one Params struct in each module. As AminoJSON supports backward compatibility (new code reads old structured data) and forward compatibility (old code reads new structured data), this works if we stick to adding fields only and maintaining their order.
Done
Done.
With the AminoJSON backward compatibility mentioned above, do we still see the benefit of keeping "params" as individual fields? The parameters in each module provide a safeguard to prevent mismatching key names between the genesis, code, GovDAO proposal. Additionally, they validate the parameter values and ensure that parameter changes are tracked in version-controlled history. Once the chain is released, and as time goes on, the individuals responsible for constructing the genesis, writing the module code, and proposing value changes through the GovDAO are likely to belong to three different groups. Without module parameters as a safeguard, imagine having more than 20 parameters—finding and matching those prefixed keys would not be an easy task if changes do not take effect. Tracing the root cause could also be time-consuming. |
Context for Locking Token Transfer
This feature allows us to lock token transfers, except for paying gas fees to add a package or call a contract. The restriction will be unlocked through GovDAO voting on a proposal.
We also want a few whitelisted, unrestricted accounts to vote on the proposal since a separate fee is required to initiate and vote on proposals.
This implementation manages the lock and unlock settings in r/sys/params, where we change the chain’s parameters. Calling lock or unlock will automatically submit a proposal to GovDAO. Once GovDAO votes and approves the proposal, the token
transfer restriction will be removed instantly.
All changes to parameters specified in r/sys/params must go through GovDAO voting.
Here are some implementation details
Main Idea Behind the Alternative Approach To implement parameter module ( Discussion)
Todo: update other params...
Contributors' checklist...