Unbonding, Redelegation #1119

rigelrozanski · 2018-06-02T01:18:41Z

Closes #992
Closes #1122

Only Active Slashing for both redelegation and unbonding

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

codecov · 2018-06-05T09:27:46Z

Codecov Report

Merging #1119 into develop will decrease coverage by 2.31%.
The diff coverage is 59.2%.

@@             Coverage Diff             @@
##           develop    #1119      +/-   ##
===========================================
- Coverage    64.47%   62.16%   -2.32%     
===========================================
  Files          112      103       -9     
  Lines         6300     5838     -462     
===========================================
- Hits          4062     3629     -433     
+ Misses        2014     1999      -15     
+ Partials       224      210      -14

cwgoes · 2018-06-26T21:26:49Z

@cwgoes yeah so I want to implement that change as a separate PR which also rips out the inflation - just the distinction between using rationals and integers to represent potentially (to small) rewards

OK; it makes #1278 a bit ugly but makes sense.

cwgoes · 2018-06-26T21:31:11Z

x/auth/ante.go

@@ -85,7 +85,7 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			signerAddr, sig := signerAddrs[i], sigs[i]

 			// check signature, return account with incremented nonce
-			signBytes := StdSignBytes(ctx.ChainID(), accNums[i], sequences[i], fee, msgs, stdTx.GetMemo())
+			signBytes := StdSignBytes(chainID, accNums[i], sequences[i], fee, msgs, stdTx.GetMemo())


Does ctx.ChainID() not work? Why is this change in this PR?

chainID comes from ctx.ChainID() - look at the few lines above - without this change signatures fail in lcd tests

Probably somewhere we aren't passing chain_id correctly in the LCD request body then? The code on develop is confusing but it doesn't actually read from Viper.

cwgoes · 2018-06-26T21:31:59Z

x/slashing/errors.go

 )

 func ErrNoValidatorForAddress(codespace sdk.CodespaceType) sdk.Error {
-	return newError(codespace, CodeInvalidValidator, "that address is not associated with any known validator")
+	return sdk.NewError(codespace, CodeInvalidValidator, "That address is not associated with any known validator")


I think we wanted lowercase error strings, nbd though

addressed all in x/slashing

cwgoes · 2018-06-26T21:35:03Z

x/stake/keeper/keeper.go

+}
+
+// return the coinkeeper
+func (k Keeper) CoinKeeper() bank.Keeper {


Do we need to expose this? Means that anyone passed stake.Keeper can arbitrarily modify account balances.

Yeah I'll modify the structure of the handler to remove this

cwgoes · 2018-06-26T21:51:11Z

client/lcd/lcd_test.go

+	validatorDstAddrBech := sdk.MustBech32ifyVal(validatorDstAddr)
+
+	// send
+	jsonStr := []byte(fmt.Sprintf(`{


Here might be where you want to pass chain_id, and set ctx.ChainID = body.ChainID in the handler

cwgoes · 2018-06-26T21:54:21Z

CHANGELOG.md

+* [cli] fixed cli-bash tests
+* [ci] added cli-bash tests
+* [basecoin] updated basecoin for stake and slashing
+* [docs] fixed references to old cli commands


Did that happen in this PR? Some of these changelog entries look unrelated

yeah not sure how that got in there, removed the duplicate lines

cwgoes · 2018-06-26T21:55:00Z

client/lcd/lcd_test.go

+		"begin_unbondings": [], 
+		"complete_unbondings": [], 
+		"begin_redelegates": [], 
+		"complete_redelegates": []


chain_id might be needed here if not specified already (can't see entire JSON struct in diff)

added, using viper to receive in the test (which needs to be changed), I think that'll be my next PR, refactor out viper usage, pretty much the same as using global variables

cwgoes · 2018-06-26T21:55:14Z

client/lcd/lcd_test.go

+		], 
+		"complete_unbondings": [], 
+		"begin_redelegates": [], 
+		"complete_redelegates": []


chain_id might be needed here (can't see entire JSON struct in diff)

same as above

cwgoes · 2018-06-26T21:59:43Z

x/stake/handler.go

-		// Update bond height
-		bond.Height = ctx.BlockHeight()
-		k.setDelegation(ctx, bond)
+	returnAmount, err := k.Unbond(ctx, msg.DelegatorAddr, msg.ValidatorSrcAddr, msg.SharesAmount)


Do we prevent transitive redelegation (redelegating from A to B, then again from B to C before the first redelegation has completed)? I think transitive redelegation will break some accounting, both here and in slashing.

…into rigel/unbonding

cwgoes

Tested ACK

Left for future PRs:

Refactoring out global pool shares (no longer necessary)
More permissive redelegation (can redelegate from any source other than a existing redelegation)
More randomized testcases!

* stake/fees spec updates * staking overview.md revisions, moving files * docs reorganization * staking spec state revisions * transaction stake updates * complete staking spec update * WIP adding unbonding/redelegation commands * added msg types for unbonding, redelegation * stake sub-package reorg * working stake reorg * modify lcd tests to not use hardcoded json strings * add description update * index keys * key managment for unbonding redelegation complete * update stake errors * completed handleMsgCompleteUnbonding fn * updated to use begin/complete unbonding/redelegation * fix token shares bug * develop docs into unbonding * got non-tests compiling after merge develop * working fixing tests * PrivlegedKeeper -> PrivilegedKeeper * tests compile * fix some tests * fixing tests * remove PrivilegedKeeper * get unbonding bug * only rpc sig verification failed tests now * move percent unbonding/redelegation to the CLI and out of handler logic * remove min unbonding height * add lcd txs * add pool sanity checks, fix a buncha tests * fix ante. set lcd log to debug (#1322) * redelegation tests, adding query functionality for bonds * add self-delegations at genesis ref #1165 * PR comments (mostly) addressed * cleanup, added Query LCD functionality * test cleanup/fixes * fix governance test * SlashValidatorSet -> ValidatorSet * changelog * stake lcd fix * x/auth: fix chainID in ante * fix lcd test * fix lint, update lint make command for spelling * lowercase error string * don't expose coinkeeper in staking * remove a few duplicate lines in changelog * chain_id in stake lcd tests * added transient redelegation * 'transient' => 'transitive' * Re-add nolint instruction * Fix tiny linter error

rigelrozanski requested a review from ebuchman as a code owner June 2, 2018 01:18

rigelrozanski added the C:x/staking label Jun 2, 2018

rigelrozanski force-pushed the rigel/unbonding branch from 159e796 to d8d7525 Compare June 2, 2018 12:11

rigelrozanski added 17 commits June 5, 2018 09:56

stake/fees spec updates

3d0e7ff

staking overview.md revisions, moving files

43a0ed9

docs reorganization

a49f9cb

staking spec state revisions

f972cab

transaction stake updates

752f084

complete staking spec update

5a77d7d

WIP adding unbonding/redelegation commands

26681a8

added msg types for unbonding, redelegation

8e677e8

stake sub-package reorg

43429ef

...

482a3c5

working stake reorg

fd1eb5f

...

63a0dcc

...

7eaa715

modify lcd tests to not use hardcoded json strings

3fac834

add description update

d69d179

index keys

0efb0a4

...

2153e0d

rigelrozanski force-pushed the rigel/unbonding branch from 7a51657 to 2153e0d Compare June 5, 2018 17:05

rigelrozanski added 8 commits June 5, 2018 15:30

...

f6b9893

key managment for unbonding redelegation complete

e460c06

...

8639f91

update stake errors

9ee70ea

...

bd41980

completed handleMsgCompleteUnbonding fn

40ad0a5

updated to use begin/complete unbonding/redelegation

4be5e41

...

03c6804

Merge remote-tracking branch 'origin/develop' into rigel/unbonding

7170bd9

cwgoes reviewed Jun 26, 2018

View reviewed changes

lowercase error string

74e0fb8

cwgoes reviewed Jun 26, 2018

View reviewed changes

rigelrozanski and others added 12 commits June 26, 2018 18:36

don't expose coinkeeper in staking

5329d73

Merge remote-tracking branch 'origin/develop' into rigel/unbonding

1a945a2

remove a few duplicate lines in changelog

ffaefec

...

0012a46

chain_id in stake lcd tests

6f2c29b

Merge branch 'develop' into rigel/unbonding

637b275

added transient redelegation

4908504

Merge branch 'rigel/unbonding' of https://github.com/cosmos/cosmos-sdk …

34e46ac

…into rigel/unbonding

'transient' => 'transitive'

f499b88

Merge branch 'develop' into rigel/unbonding

bcace44

Re-add nolint instruction

de77006

Fix tiny linter error

791ad9c

cwgoes approved these changes Jun 27, 2018

View reviewed changes

cwgoes merged commit 6f140d7 into develop Jun 27, 2018

ValarDragon mentioned this pull request Jun 27, 2018

Keys are declared as var and exposed #1261

Closed

rigelrozanski deleted the rigel/unbonding branch June 27, 2018 18:48

cwgoes mentioned this pull request Jun 27, 2018

Ledger integration #931

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unbonding, Redelegation #1119

Unbonding, Redelegation #1119

rigelrozanski commented Jun 2, 2018 •

edited

Loading

codecov bot commented Jun 5, 2018 •

edited

Loading

cwgoes commented Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

cwgoes Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

rigelrozanski Jun 26, 2018

cwgoes Jun 26, 2018

cwgoes left a comment •

edited

Loading

Unbonding, Redelegation #1119

Unbonding, Redelegation #1119

Conversation

rigelrozanski commented Jun 2, 2018 • edited Loading

codecov bot commented Jun 5, 2018 • edited Loading

Codecov Report

cwgoes commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

rigelrozanski commented Jun 2, 2018 •

edited

Loading

codecov bot commented Jun 5, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading