[action] New MigrateStake Action #4299

envestcc · 2024-06-11T14:47:25Z

Description

Introduce the new action MigrateStake to migrate a native bucket to nft bucket with same stake amount and duration

based on #4305 , iotexproject/iotex-proto#153

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[] make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

CoderZhi · 2024-06-12T00:00:23Z

action/protocol/staking/handler_stake_migrate.go

+)
+
+const (
+	executionProtocolID = "smart_contract"


execution.Name()

import execution pkg will cause cycle-dependency

CoderZhi · 2024-06-12T00:01:16Z

action/protocol/staking/handler_stake_migrate.go

+)
+
+type (
+	executionProtocol interface {


no difference from protocol.ActionHandler

changed to call HandleCrossProtocol, which will reset caller nonce, avoiding nonce set error

CoderZhi · 2024-06-12T00:04:44Z

action/protocol/staking/handler_stake_migrate.go

+		return nil, nil, 0, errors.Wrap(err, "failed to handle execution action")
+	}
+	if excReceipt.Status != uint64(iotextypes.ReceiptStatus_Success) {
+		// TODO: return err or handle error?


what kind of error could be handled?

I'm not sure whether return error or receipt with faliure status if contract call status is not success, but we can keep it simply to return error

CoderZhi · 2024-06-12T00:06:04Z

action/protocol/staking/handler_stake_migrate.go

+	// add sub-receipts logs
+	actLogs = append(actLogs, excReceipt.Logs()...)
+	transferLogs = append(transferLogs, excReceipt.TransactionLogs()...)
+	return actLogs, transferLogs, excReceipt.GasConsumed, nil


gasConsumed + 10000, 10000 is the base fee to withdraw a native bucket

CoderZhi · 2024-06-12T00:06:50Z

action/protocol/staking/handler_stake_migrate.go

+	if err := validateBucketOwner(bucket, protocol.MustGetActionCtx(ctx).Caller); err != nil {
+		return err
+	}
+	if !bucket.AutoStake {


what's the concern?

If unlocked is permitted, an additional unlock contract call is required, which may makes the code more complex.

CoderZhi · 2024-06-12T00:08:47Z

action/protocol/staking/handler_stake_migrate.go

+			failureStatus: iotextypes.ReceiptStatus_ErrNotEnoughBalance,
+		}
+	}
+	// clear candidate's self stake if the


self stake bucket cannot be migrated

sure, here is to clear stale status(e.g. endorsement has been expired)

CoderZhi · 2024-06-12T00:09:39Z

action/protocol/staking/handler_stake_migrate.go

+	// delete bucket and bucket index
+	if err := csm.delBucketAndIndex(bucket.Owner, bucket.Candidate, bucket.Index); err != nil {
+		return nil, nil, errors.Wrapf(err, "failed to delete bucket for candidate %s", bucket.Candidate.String())
+	}
+
+	// update bucket pool
+	if err := csm.CreditBucketPool(bucket.StakedAmount); err != nil {
+		return nil, nil, errors.Wrapf(err, "failed to update staking bucket pool %s", err.Error())
+	}
+	// update candidate vote
+	weightedVote := p.calculateVoteWeight(bucket, false)
+	if err := cand.SubVote(weightedVote); err != nil {
+		return nil, nil, &handleError{
+			err:           errors.Wrapf(err, "failed to subtract vote for candidate %s", bucket.Candidate.String()),
+			failureStatus: iotextypes.ReceiptStatus_ErrNotEnoughBalance,
+		}
+	}
+	// clear candidate's self stake if the
+	if cand.SelfStakeBucketIdx == bucket.Index {
+		cand.SelfStake = big.NewInt(0)
+		cand.SelfStakeBucketIdx = candidateNoSelfStakeBucketIndex
+	}
+	if err := csm.Upsert(cand); err != nil {
+		return nil, nil, csmErrorToHandleError(cand.GetIdentifier().String(), err)
+	}


can we wrap it into a function defined in csm?

CoderZhi · 2024-06-12T00:18:47Z

action/protocol/staking/handler_stake_migrate.go

+	return validateBucketSelfStake(protocol.MustGetFeatureCtx(ctx), csm, bucket, false)
+}
+
+func (p *Protocol) withdrawBucket(ctx context.Context, withdrawer *state.Account, bucket *VoteBucket, cand *Candidate, csm CandidateStateManager) (*receiptLog, *action.TransactionLog, error) {


share with withdraw bucket action

This may not apply to handle withdraw action, as it involves the reduction of votes which is handled in unstake action

action/protocol/staking/handler_stake_migrate.go

CoderZhi · 2024-06-12T00:23:30Z

action/protocol/staking/handler_stake_migrate.go

+	}
+	exeAct, err := action.NewExecution(
+		contractAddress,
+		act.Nonce(),


is nonce important? if not, can we leave it as 0?

It's critical to SetNonce of the caller,

CoderZhi · 2024-06-12T18:26:04Z

action/protocol/execution/protocol.go

@@ -83,6 +84,25 @@ func (p *Protocol) Handle(ctx context.Context, act action.Action, sm protocol.St
 	return receipt, nil
 }

+// HandleCrossProtocol handles an execution from another protocol
+func (p *Protocol) HandleCrossProtocol(ctx context.Context, act action.Action, sm protocol.StateManager) (*action.Receipt, error) {


We may have a function for this protocol only as

return p.handle(ctx, exec, sm) }

and this function could be reused by Handle

CoderZhi · 2024-06-12T18:33:50Z

action/protocol/staking/protocol.go

@@ -427,22 +439,25 @@ func (p *Protocol) handle(ctx context.Context, act action.Action, csm CandidateS
 		rLog, tLogs, err = p.handleCandidateEndorsement(ctx, act, csm)
 	case *action.CandidateTransferOwnership:
 		rLog, tLogs, err = p.handleCandidateTransferOwnership(ctx, act, csm)
+	case *action.MigrateStake:
+		logs, tLogs, gasConsumed, err = p.handleStakeMigrate(ctx, act, csm)


settleAction and skip setting nonce

CoderZhi · 2024-06-12T18:34:42Z

action/protocol/execution/protocol.go

+	// reset caller nonce
+	acc, err := accountutil.AccountState(ctx, sm, protocol.MustGetActionCtx(ctx).Caller)
+	if err != nil {
+		log.L().Panic("failed to get account state", zap.Error(err))
+	}
+	acc.DecreaseNonce()
+	if err := accountutil.StoreAccount(sm, protocol.MustGetActionCtx(ctx).Caller, acc); err != nil {
+		log.L().Panic("failed to store account", zap.Error(err))
+	}


thus, we can skip nonce reset

action/protocol/staking/handler_stake_migrate.go

dustinxie · 2024-06-18T05:18:30Z

action/protocol/staking/protocol.go

+	return p.handle(ctx, act, csm)
+}
+
+func (p *Protocol) constructCandidateStateManager(ctx context.Context, sm protocol.StateManager) (CandidateStateManager, error) {


why need this change? seems no real change instead of just adding this new func?

it's reused in two places

dustinxie · 2024-06-18T05:22:42Z

action/protocol/staking/protocol.go

@@ -433,22 +453,29 @@ func (p *Protocol) handle(ctx context.Context, act action.Action, csm CandidateS
 		rLog, tLogs, err = p.handleCandidateEndorsement(ctx, act, csm)
 	case *action.CandidateTransferOwnership:
 		rLog, tLogs, err = p.handleCandidateTransferOwnership(ctx, act, csm)
+	case *action.MigrateStake:
+		var nonceUpdated bool
+		logs, tLogs, gasConsumed, nonceUpdated, err = p.handleStakeMigrate(ctx, act, csm)


why not follow other actions rLog, tLogs ... =

rLog can only represent log for one address, whereas here it will include logs for multiple addresses (i.e. NativeStakingAddress and ContractStakingAddress).

dustinxie · 2024-06-18T05:24:14Z

action/protocol/staking/protocol.go

-
-	if l := rLog.Build(ctx, err); l != nil {
-		logs = append(logs, l)
+	if rLog != nil {


and use the same rLog.Build() here?

as explained above

dustinxie · 2024-06-18T05:34:58Z

action/protocol/staking/protocol.go

+const (
+	updateNonce   nonceUpdateType = true
+	noUpdateNonce nonceUpdateType = false
+)


feel there's no need for this, using bool is enough, the following logic should be clear

if nonceUpdated { // updated in createNFTBucket, no need to update again in settleAction nonceUpdated = false }

dustinxie · 2024-06-18T05:42:40Z

action/protocol/staking/protocol.go

-	}
-	if err := accountutil.StoreAccount(sm, actionCtx.Caller, acc); err != nil {
-		return nil, errors.Wrap(err, "failed to update nonce")
+	if updateNonce {


so if thecreateNFTBucket executed successfully, the nonce is increased by EVM, then we don't increase nonce again? would that be considered the same nonce (actionCtx.Nonce + 1) did 2 actions: withdraw bucket + create NFT bucket?

This is only one action, not two from user perspective , so we should set nonce+1

dustinxie · 2024-06-18T05:44:18Z

action/protocol/staking/protocol.go

 	}
 	r := action.Receipt{
 		Status:          status,
 		BlockHeight:     blkCtx.BlockHeight,
 		ActionHash:      actionCtx.ActionHash,
-		GasConsumed:     actionCtx.IntrinsicGas,
+		GasConsumed:     gasConsumed,


should still use IntrinsicGas? the EVM part gas is consumed inside EVM?

gasConsumed of Receipt should represent the total gas consumed in the StakeMigrate action, so it should include gas in evm

CoderZhi · 2024-06-18T07:18:02Z

action/protocol/staking/handler_stake_migrate.go

 	}
 	// add sub-receipts logs
 	actLogs = append(actLogs, excReceipt.Logs()...)
 	transferLogs = append(transferLogs, excReceipt.TransactionLogs()...)
-	return actLogs, transferLogs, excReceipt.GasConsumed + insGas, nonceUpdated, nil
+	return actLogs, transferLogs, insGas, nonceUpdated, nil


return actLogs, transferLogs, insGas, true, nil
it seems that if err != nil, nonceUpdated is always false, else it is always true.

yeah, seems like nonceUpdated is not necessary, and just not update nonce again if returned error == nil

CoderZhi · 2024-06-18T07:22:16Z

action/protocol/staking/handler_stake_migrate_test.go

@@ -284,7 +310,7 @@ func TestHandleStakeMigrate(t *testing.T) {
 		r.Equal(uint64(iotextypes.ReceiptStatus_Success), receipts[0].Status)
 		// gas = instrinsic  + contract call
 		instriGas, _ := act.IntrinsicGas()
-		r.Equal(receipt.GasConsumed+instriGas, receipts[0].GasConsumed)
+		r.Equal(instriGas, receipts[0].GasConsumed)


receipts[0].GasConsumed should be intrinsicGas + execution.GasConsumed.

CoderZhi · 2024-06-18T07:27:21Z

action/protocol/staking/protocol.go

 ) (*action.Receipt, error) {
 	actionCtx := protocol.MustGetActionCtx(ctx)
 	blkCtx := protocol.MustGetBlockCtx(ctx)
-	gasFee := big.NewInt(0).Mul(actionCtx.GasPrice, big.NewInt(0).SetUint64(actionCtx.IntrinsicGas))
+	gasFee := big.NewInt(0).Mul(actionCtx.GasPrice, big.NewInt(0).SetUint64(gasConsumed))


gas reported in receipt is different from the gas to be charged. the gas to be charged here is an extra gas

indeed, has separated into the gasConsumed and gasToBeDeducted

CoderZhi · 2024-06-18T07:32:31Z

e2etest/native_staking_test.go

+				expect: []actionExpect{&basicActionExpect{nil, uint64(iotextypes.ReceiptStatus_ErrUnauthorizedOperator), ""}},
+			},
+			{
+				name:   "success to migrate stake",


the e2e test doesn't covered the gas consumption calculation, which should be:
the balance diff of the sender account is equal to intrinsic gas + evm gas, and the value is also identical to receipt.GasConsumed

CoderZhi · 2024-06-18T07:49:26Z

action/protocol/staking/protocol.go

+	case *action.MigrateStake:
+		var nonceUpdated bool
+		logs, tLogs, gasConsumed, nonceUpdated, err = p.handleStakeMigrate(ctx, act, csm)
+		if nonceUpdated {


if err == nil { nonceUpdateOption = noUpdateNonce }

dustinxie · 2024-06-18T23:03:35Z

action/protocol/execution/protocol_test.go

@@ -3,7 +3,7 @@
 // or fitness for purpose and, to the extent permitted by law, all liability for your use of the code is disclaimed.
 // This source code is governed by Apache License 2.0 that can be found in the LICENSE file.

-package execution
+package execution_test


why need to change this

to solve import cycle issue between execution and staking

dustinxie · 2024-06-18T23:20:23Z

action/rlp_tx_test.go

+			),
+			action:  MustNoErrorV(NewMigrateStake(nonce, 1, gasLimit, gasPrice)),
+			builder: elpbuilder.BuildStakingAction,
+		},


add a testcase in TestEthTxDecodeVerify, which tests more

dustinxie · 2024-06-18T23:28:21Z

action/protocol/staking/handler_stake_migrate.go

+
+func (p *Protocol) ConstructExecution(ctx context.Context, act *action.MigrateStake, sr protocol.StateReader) (*action.Execution, error) {
+	sm := protocol.NewStateManagerWrapper(sr)
+	csm, err := p.constructCandidateStateManager(ctx, sm)


i think all you need is:

csr, err := ConstructBaseView(sr)

so we can remove all the constructCandidateStateManager and NewStateManagerWrapper code

and with this change:

func (p *Protocol) fetchBucket(bucketGetter BucketGetByIndex, index uint64) (*VoteBucket, ReceiptError) {

dustinxie · 2024-06-18T23:39:34Z

action/protocol/staking/handler_stake_migrate.go

+}
+
+func (p *Protocol) createNFTBucket(ctx context.Context, exeAct *action.Execution, sm protocol.StateManager) (*action.Receipt, error) {
+	exctPtl := execution.FindProtocol(protocol.MustGetRegistry(ctx))


this part can move to Start() to get the exctPtl and keep it, and directly use it here

dustinxie · 2024-06-18T23:40:11Z

action/protocol/managers.go

+	// until the candidate state manager and candidate state reader are refactored
+	StateManagerWrapper struct {
+		StateReader
+	}


no needed, so comments above

dustinxie · 2024-06-19T00:07:04Z

action/protocol/staking/handler_stake_migrate.go

+	// force-withdraw native bucket
+	actLog, tLog, err := p.withdrawBucket(ctx, staker, bucket, candidate, csm)
+	if err != nil {
+		return nil, nil, gasConsumed, gasToBeDeducted, err


no need to revertSM() here?
if the last error to put action caller into SM failed, it won't cause change to SM?

dustinxie · 2024-06-19T00:13:58Z

action/protocol/staking/handler_stake_migrate.go

+		return nil, nil, 0, 0, err
+	}
+	gasConsumed := insGas
+	gasToBeDeducted := insGas


group these 2 and above 2 logs into

var ( )

sonarqubecloud · 2024-06-19T02:36:04Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.8% Duplication on New Code

See analysis details on SonarCloud

envestcc changed the title ~~[action] New StakeMigrate Action~~ [action] New MigrateStake Action Jun 11, 2024

CoderZhi reviewed Jun 12, 2024

View reviewed changes

envestcc force-pushed the native-migration branch from bd17ea8 to 0ca40b9 Compare June 12, 2024 14:48

CoderZhi reviewed Jun 12, 2024

View reviewed changes

envestcc force-pushed the native-migration branch 2 times, most recently from bb6f4cd to 45ffc14 Compare June 17, 2024 03:47

envestcc marked this pull request as ready for review June 17, 2024 04:11

envestcc requested review from dustinxie, Liuhaai and a team as code owners June 17, 2024 04:11

envestcc force-pushed the native-migration branch 2 times, most recently from 22a299c to 65d977d Compare June 17, 2024 04:30

envestcc mentioned this pull request Jun 17, 2024

[staking] enable new staking index #4305

Merged

1 task

envestcc force-pushed the native-migration branch 2 times, most recently from 920381f to 072f6d8 Compare June 18, 2024 01:31

envestcc commented Jun 18, 2024

View reviewed changes

action/protocol/staking/handler_stake_migrate.go Outdated Show resolved Hide resolved

envestcc commented Jun 18, 2024

View reviewed changes

action/protocol/staking/handler_stake_migrate.go Show resolved Hide resolved

envestcc commented Jun 18, 2024

View reviewed changes

action/protocol/staking/handler_stake_migrate.go Outdated Show resolved Hide resolved

dustinxie reviewed Jun 18, 2024

View reviewed changes

envestcc and others added 6 commits June 18, 2024 13:44

migrate stake

fec103c

fix migrate stake

dd78e46

add gas estimation for stake migration

ea134b9

fix migration bug

81de2a4

fix migration topic

300bfde

update go.mod

191c710

envestcc and others added 10 commits June 18, 2024 13:45

add e2etest for migrate

374f89e

add test

7dbd5bd

resolve the loop dependency

28978f9

fix test

e4d8cca

fix test

e52cecf

not set nonce in staking

9b8fccc

fix rebase

6d9b054

fix test

c44e2a4

add action test

0fda2c4

address comments

e81cb70

envestcc force-pushed the native-migration branch from aa13852 to e81cb70 Compare June 18, 2024 05:48

CoderZhi reviewed Jun 18, 2024

View reviewed changes

envestcc added 3 commits June 18, 2024 17:01

fix gas

d5d268f

address comment

45ac052

fix e2etest

889eaf5

CoderZhi approved these changes Jun 18, 2024

View reviewed changes

dustinxie reviewed Jun 18, 2024

View reviewed changes

dustinxie reviewed Jun 19, 2024

View reviewed changes

address comment

a8923f0

dustinxie approved these changes Jun 19, 2024

View reviewed changes

envestcc merged commit 5a6871d into master Jun 19, 2024
3 checks passed

envestcc deleted the native-migration branch June 19, 2024 03:30

envestcc mentioned this pull request Oct 19, 2024

Native bucket migrate to NFT #3978

Closed

[action] New MigrateStake Action #4299

[action] New MigrateStake Action #4299

Conversation

envestcc commented Jun 11, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

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

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

envestcc Jun 18, 2024 • edited Loading

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

dustinxie Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

dustinxie Jun 18, 2024 • edited Loading

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

sonarqubecloud bot commented Jun 19, 2024

Quality Gate passed

envestcc commented Jun 11, 2024 •

edited

Loading

envestcc Jun 18, 2024 •

edited

Loading

dustinxie Jun 18, 2024 •

edited

Loading

dustinxie Jun 18, 2024 •

edited

Loading