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

go staking: disable all transactions for locked accounts #2727

Closed
wants to merge 5 commits into from

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Feb 26, 2020

The account field general.transfers_not_before is replaced with general.not_before. Accounts can't send any transaction before the epoch in that field.

@pro-wh pro-wh added c:consensus/cometbft Category: CometBFT c:staking Category: staking labels Feb 26, 2020
@pro-wh pro-wh force-pushed the pro-wh/feature/notbefore branch from e93f8ae to 913bd31 Compare February 26, 2020 00:41
@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #2727 into master will decrease coverage by 0.24%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2727      +/-   ##
==========================================
- Coverage   63.22%   62.97%   -0.25%     
==========================================
  Files         378      378              
  Lines       35532    35539       +7     
==========================================
- Hits        22464    22382      -82     
- Misses      10248    10342      +94     
+ Partials     2820     2815       -5
Impacted Files Coverage Δ
go/consensus/api/transaction/transaction.go 63.01% <ø> (ø) ⬆️
go/staking/api/api.go 71.42% <ø> (ø) ⬆️
go/consensus/tendermint/abci/context.go 76.72% <0%> (-1.35%) ⬇️
go/common/errors/errors.go 100% <100%> (ø) ⬆️
go/consensus/tendermint/apps/staking/state/gas.go 91.48% <100%> (+1.48%) ⬆️
go/consensus/tendermint/apps/staking/auth.go 69.23% <50%> (-10.77%) ⬇️
.../consensus/tendermint/apps/staking/transactions.go 76.58% <66.66%> (+0.39%) ⬆️
go/genesis/api/api.go 53.33% <0%> (-20.01%) ⬇️
go/worker/common/host/interface.go 38.46% <0%> (-15.39%) ⬇️
go/consensus/tendermint/api/api.go 73.58% <0%> (-15.1%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66d5a62...dba471f. Read the comment docs.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

This approach seems error-prone as it is easy to forget a place where general balance is used. Wouldn't it be cleaner to have the locked funds in a separate Locked subaccount and then have an operation to move the funds to the general balance?

The operation for unlocking could be a self-transfer, or (IMO nicer as it is more explicit) a special transaction. Of course introducing a new staking transaction will have some people cry "but but Ledger" but support is not actually needed until the first unlock period expires which should be some time out.

go/common/errors/errors.go Outdated Show resolved Hide resolved
@@ -77,12 +77,14 @@ type Context struct {
}

// NewMockContext creates a new mock context for use in tests.
func NewMockContext(mode ContextMode, now time.Time) *Context {
func NewMockContext(mode ContextMode, now time.Time, state *iavl.MutableTree) *Context {
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this now that we have mock application state (I actually removed it in #2674). Can you use NewMockApplicationState in tests instead as it already handles creating a new in-memory tree for you?

@pro-wh
Copy link
Contributor Author

pro-wh commented Feb 26, 2020

could we ditch this and program in the locked funds in terms of debonding entries?

@pro-wh
Copy link
Contributor Author

pro-wh commented Feb 28, 2020

^ #2747

@pro-wh
Copy link
Contributor Author

pro-wh commented Mar 5, 2020

we'll go without this feature as implemented. the current idea is to put locked tokens in debonding delegations at genesis. we added a test for this use case in #2747

@pro-wh pro-wh closed this Mar 5, 2020
@pro-wh pro-wh deleted the pro-wh/feature/notbefore branch March 5, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:consensus/cometbft Category: CometBFT c:staking Category: staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants