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

feat: restrict sending gnot by default #2667

Closed
wants to merge 16 commits into from

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Aug 7, 2024

NOTE: I've just seen the comment from @moul on the original issue related to this, so parts of this solution may need to be reworked -- #1837 (comment)

BREAKING CHANGE in the sense that GNOT will no longer be transferable by default unless otherwise specified when starting the node.

This PR aims to do the following:

  • Disable explicit GNOT transfers by default (not default for gnodev and txtar tests)
  • GNOT can only be transferred to pay gas fees

This is the first of two PRs. The next will be to add a mechanism to unlock GNOT transfer restrictions on a per account basis by the account that wishes to be unlocked broadcasting a new type of transaction.

I'm seeking feedback on this before merging, mainly to get some clarification on the following questions:

  • Should locking be enabled by default? I think it makes sense, as genesis can be modified or a flag can be specified when starting a node when doing lazy genesis initialization.
  • Should there also be fields in genesis that are exempt from the restrictions, such as some system account owned by us? The subsequent unlocking PR will also likely include "semi-exempt" accounts as genesis fields -- these being accounts that are exempt in the sense that they are able to submit a transaction to unlock their account. Genesis will also have a field specifying whether or not accounts are generally unlockable or not. A chain upgrade would be required to make accounts generally unlockable after initial launch

No docs will be updated as a part of this PR until there has been more of a discussion.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 7, 2024
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 65.33333% with 26 lines in your changes missing coverage. Please review.

Project coverage is 60.15%. Comparing base (bbcb2f6) to head (e364c6d).
Report is 92 commits behind head on master.

Files with missing lines Patch % Lines
gno.land/pkg/gnoland/app.go 0.00% 7 Missing ⚠️
tm2/pkg/std/coin.go 0.00% 5 Missing ⚠️
gno.land/cmd/gnoland/start.go 71.42% 3 Missing and 1 partial ⚠️
tm2/pkg/sdk/bank/keeper.go 82.60% 3 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_generate.go 81.81% 1 Missing and 1 partial ⚠️
gno.land/pkg/integration/testing_integration.go 0.00% 1 Missing and 1 partial ⚠️
tm2/pkg/std/account.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2667      +/-   ##
==========================================
+ Coverage   60.11%   60.15%   +0.04%     
==========================================
  Files         560      561       +1     
  Lines       74918    75061     +143     
==========================================
+ Hits        45039    45155     +116     
- Misses      26504    26525      +21     
- Partials     3375     3381       +6     
Flag Coverage Δ
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
gno.land 64.69% <55.88%> (+0.52%) ⬆️
gnovm 64.13% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (+0.35%) ⬆️
tm2 62.00% <71.79%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gnoland start --ugnot-locked

! gnokey maketx send -send "9999999ugnot" -to g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5 -gas-fee 1ugnot -gas-wanted 10000000 -broadcast -chainid=tendermint_test test1
stderr 'account is locked'
Copy link
Member

@moul moul Aug 29, 2024

Choose a reason for hiding this comment

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

"ugnot transfer is locked for this account or globally" (the account it not locked)

@moul
Copy link
Member

moul commented Aug 29, 2024

Sorry, I just left one comment, but I think before reviewing individual lines, we need:

  1. To clarify the overall strategy.
  2. To ensure the names will be good enough, especially if we want to make changes in TM2 and not only gno.land.

Braindump:

  • We need a way to lock GNO token transfer globally, which can be changed by a DAO, and a way to enforce whitelist exceptions (e.g., faucets) during that period.
  • We could also need or want another layer of whitelisting, where this time, it won't be an exception during global lock, but something that will only works when token transfers are globally unlocked. This would allow differentiating accounts that have "signed the T.O.S/EULA/whatever" from non-signers.
  • In terms of implementation, we have already identified other needs that may require a way to set a global flag for the chain from a contract, which can be read (for cheap) by TM2. This introduces the notion of adding a std.SetConfig(key, value string) function, where the chain and eventually other contracts could call std.GetConfig(realm, key string) string. Alternatively, we could rely on modules where gno.land will load the GNO VM to call a contract helper, as done for instance in feat: add valset injection through r/sys/validators #2229.
  • this global mechanism could potentially be used not only for global variables, but also for account-specific or realm-specific variables, by just prefixing the key with what we're looking for, i.e., gno.land/r/sys/tos.SetConfig("signed-by-<std.GetOrigCaller()>")
  • We're considering extending the account store. Here, we have two main location options: one is the TM2 store itself, and another could be specific to gno.land. If we want to use the TM2 store, there's a question about minimalism and staying generalist that we may eventually address by adding optional columns that can be used by gnovm-based chains to add new fields on the account (i.e., USER1, USER2, USER3 in linux processes), and then add some helpers or pass an account keeper to apply the changes. If we prefer using a dedicated gno.land store for users, then we have a constraint of performance, where ideally, we should do a single store read query and not start having two.

@deelawn deelawn closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants