[feat] Resubmit the operator module #12

TimmyExogenous · 2024-03-08T01:52:41Z

Description

This PR adds an implementation designed in the below file.
The state update of undelegation and bonding assets

This PR introduces interfaces for recording and updating asset amounts and shares in response to external events, such as delegation/undelegation, opt-in/opt-out, and slashing. The asset shares can also be utilized to calculate the voting power of both stakers and operators. Subsequently, the reward module can calculate rewards based on the computed voting power.
Additionally, this PR introduces a scheduling function to manage the changes in asset shares caused by price fluctuation.
All logic has been implemented in a new module named 'operator' in this PR.

The core logic is located in the following files:
/x/operator/state_update.go
/x/operator/abci.go

The provided interfaces are as below:

UpdateOptedInAssetsState(ctx sdk.Context, stakerId, assetId, operatorAddr string, opAmount sdkmath.Int) error

OptIn(ctx sdk.Context, operatorAddress sdk.AccAddress, AVSAddr string) error

OptOut(ctx sdk.Context, OperatorAddress sdk.AccAddress, AVSAddr string) error

Slash(ctx sdk.Context, operatorAddress sdk.AccAddress, AVSAddr, slashContract, slashId string, occurredSateHeight int64, slashProportion sdkmath.LegacyDec) error

The implementation requires certain interfaces from the Oracle and AVS modules. It needs to retrieve price changes from the Oracle module and access the assets supported by AVS to update the asset shares.

Except for the features mentioned above, this new PR adds the functions expected by the dogfood module. These functions have been implemented in the app-chain module by Max. The related PR is as follows:
app-chain PR

This PR also resolves some problems that would cause lint errors.
Supersedes old PR about operator module from the older repo.

Todo

code comments and tests

The interface required by IBC was already implemented in `validators.go`. This PR takes that a step further and implements the interfaces required to use the dogfood module as a drop-in replacement for the SDK's staking module. The interfaces implemented are those expected by the slashing, evidence and `genutil` modules. An explanation has been provided above each function to explain why and where it is called, and if its implementation is really necessary. There are still some TODOs left in this PR that depend on the overall slashing / operator design. This branch is merged into dogfood-part6, which forms the basis of #122.

fix the bug caused by rebase merge remove license comments add some code comments change precompile contract solidity version

…ule.

TimmyExogenous · 2024-03-12T07:27:27Z

All problems identified by code review have been resolved.

proto/exocore/assets/v1/query.proto

proto/exocore/assets/v1/tx.proto

x/assets/keeper/operator_asset.go

x/assets/types/errors.go

x/assets/types/keys.go

x/delegation/keeper/keeper.go

x/delegation/types/keys.go

…le and delete redudant code rename chainId to chainID in dogfood module rename restakingKeeper to assetsKeeper

TimmyExogenous · 2024-03-13T06:36:42Z

All problems have been resolved. Additionally, the code related to setting ExocoreLzAppAddress has been refined.

MaxMustermann2

There are still some unresolved conversations associated with prior reviews. Please resolve them or comment on them if you won't be fixing them.

precompiles/deposit/deposit_test.go

precompiles/deposit/error.go

proto/exocore/assets/v1/query.proto

proto/exocore/assets/v1/tx.proto

x/operator/keeper/keeper.go

x/operator/keeper/state_update.go

…pile

precompiles/delegation/error.go

x/operator/types/expected_keepers.go

MaxMustermann2 · 2024-03-16T07:55:12Z

x/operator/keeper/dogfood.go

+
+// IsOperatorOptingOutFromChainID returns true if the operator is opting out from the given
+// chain id.
+func (k *Keeper) IsOperatorOptingOutFromChainID(


Not a review comment, but in my PR I will remove this function and change the language around these in the next PR such that opt out completes immediately for the operator module.

Okay, thanks for your great work.

MaxMustermann2 · 2024-03-17T10:08:56Z

x/delegation/keeper/abci.go

-			TotalAmountOrWantChangeValue:            actualCanUndelegateAmount.Neg(),
-			WaitUndelegationAmountOrWantChangeValue: recordAmountNeg,
+		err = k.assetsKeeper.UpdateOperatorAssetState(ctx, operatorAccAddress, record.AssetID, types.OperatorSingleAssetChangeInfo{
+			WaitUnbondingAmount: recordAmountNeg,
 		})
 		if err != nil {
 			panic(err)


Two points here:

Please remove the panics and use a cached context instead. If there is no error, the cached context's writeFunc should be called.

Add a TODO here to say that the message should be sent to the client chain to release the assets for withdrawal. I spoke to @adu-web3 who said that the funds are made available for withdrawal after the undelegation is processed by Exocore. However, that is not the correct approach since the assets are locked for a 10-block (or larger) period during which they can be slashed.

Please remove the panics and use a cached context instead. If there is no error, the cached context's writeFunc should be called.

The handle of errors in BeginBlock and EndBlock definitely needs to be reconsidered. What's the meaning of using a cached context? This can be changed in the future PR.

Add a TODO here to say that the message should be sent to the client chain to release the assets for withdrawal. I spoke to @adu-web3 who said that the funds are made available for withdrawal after the undelegation is processed by Exocore. However, that is not the correct approach since the assets are locked for a 10-block (or larger) period during which they can be slashed.

I remember the previously designed approach was that after undelegation is completed, the user's withdrawable status will increase. Exocore does not need to actively send notifications to the client chain about the completion of undelegation. The client chain will periodically or when the user initiates a transaction, synchronize the withdrawable status from Exocore to the client chain. Additionally, the code here is executed when the unbonding time expires, which means the related assets won't be slashed after this block.

For the first point: understood, we will leave this for a future PR. A cached context allows for error handling within the SDK while not modifying the state.

cachedCtx, writeCache := ctx.CacheContext() if err := k.FunctionThatMightError(ctx); err != nil { return err } // dump events ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) // store the state change from the function writeCache()

For the second point: The current implementation, in my understanding of the smart contract, is that whenever an undelegation request is executed on the client chain, a message reaches the Exocore chain. The message is executed in the ExocoreGateway.sol, which calls the undelegation precompile. The call's result (success or failure) is sent back to the client chain gateway. The client chain gateway then allows withdrawal of the undelegated assets, which is an incorrect implementation. The undelegated assets should remain stuck for 10 blocks (or the unbonding period, whichever is higher).

To fix this, my approach is to remove the immediate release feature from the client chain gateway, and for Exocore chain to send a message to the client chain when the assets are available. At this point, we may have to consider batching such messages. This is the TODO I was asking you to add here; however, it is not a blocker for this PR.

When the undelegation call's result is sent back to the client chain, It's just a message to notify the client chain that the undelegation has been successfully routed and queued in the Exocore chain. It doesn't mean the asset related to this undelegation can be withdrawn because the withdrawable amount increases only when this undelegation is completed. As for how to notify the client chain after completing the undelegation, the client chain contract will sync the withdrawable state from the Exocore chain regularly or when a staker initiates actions(such as deposit, delegation, undelegation and withdrawing) from the client chain. If you find the withdrawable amount will increase immediately when
the undelegation call's result is successfully sent back to the client chain, there might be a bug in the client chain contract. Please let me know about it.

Appreciate for providing the cache context code.

I misunderstood the Solidity implementation; please ignore the second part of my original comment.

This comment is still left unresolved intentionally to remember the panic replacement.

…ome comments and refine the naming

MaxMustermann2

Great job surviving through this long review process, I appreciate your patience!

There are a couple of questions / comments that I have left unresolved for future reference; they do not need to be answered or resolved now.

MaxMustermann2 · 2024-03-19T15:46:20Z

x/delegation/keeper/abci.go

-			TotalAmountOrWantChangeValue:            actualCanUndelegateAmount.Neg(),
-			WaitUndelegationAmountOrWantChangeValue: recordAmountNeg,
+		err = k.assetsKeeper.UpdateOperatorAssetState(ctx, operatorAccAddress, record.AssetID, types.OperatorSingleAssetChangeInfo{
+			WaitUnbondingAmount: recordAmountNeg,
 		})
 		if err != nil {
 			panic(err)


For the first point: understood, we will leave this for a future PR. A cached context allows for error handling within the SDK while not modifying the state.

cachedCtx, writeCache := ctx.CacheContext() if err := k.FunctionThatMightError(ctx); err != nil { return err } // dump events ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) // store the state change from the function writeCache()

For the second point: The current implementation, in my understanding of the smart contract, is that whenever an undelegation request is executed on the client chain, a message reaches the Exocore chain. The message is executed in the ExocoreGateway.sol, which calls the undelegation precompile. The call's result (success or failure) is sent back to the client chain gateway. The client chain gateway then allows withdrawal of the undelegated assets, which is an incorrect implementation. The undelegated assets should remain stuck for 10 blocks (or the unbonding period, whichever is higher).

To fix this, my approach is to remove the immediate release feature from the client chain gateway, and for Exocore chain to send a message to the client chain when the assets are available. At this point, we may have to consider batching such messages. This is the TODO I was asking you to add here; however, it is not a blocker for this PR.

MaxMustermann2 · 2024-03-19T15:49:43Z

x/operator/keeper/abci.go

@@ -0,0 +1,135 @@
+package keeper


Has this approach been discussed with RJ? Updating each block is very cost heavy.

MaxMustermann2 · 2024-03-19T15:51:26Z

x/operator/keeper/state_update.go

+			}
+		}
+	}
+	return nil


I think we should work on this design.

MaxMustermann2 · 2024-03-19T15:57:22Z

x/operator/types/expected_keepers.go

+	usdtAssetID := "0xdac17f958d2ee523a2206206994597c13d831ec7_0x65"
+	ret[usdtAssetID] = &PriceChange{
+		NewPrice:      sdkmath.NewInt(1),
+		OriginalPrice: sdkmath.NewInt(1),


Based on the current usage of this function, it appears to expect the price change to be as of each block. The oracle module's implementation may cause some of this to change.

…roto file

adu-web3 · 2024-03-20T07:03:15Z

The message is executed in the ExocoreGateway.sol, which calls the undelegation precompile. The call's result (success or failure) is sent back to the client chain gateway. The client chain gateway then allows withdrawal of the undelegated assets, which is an incorrect implementation

@MaxMustermann2 this is not the case for current implementation, every withdrawal must be processed by Exocore network so client chain contracts can not unlock the asset and allow for further claim by itself. For undelegation workflow, the call's result of ExocoreGateway could only indicate whether the undelegation is queued to be finished or not. We could change the result's name in contract to eliminate misunderstanding

leonz789 · 2024-03-20T07:13:08Z

ginkgo used across all _test file, but it seems only used for evmos/inflation module's integration test, all files imported ginkgo here actually not use it(no any Describe or other ginkgo test usage). Is there any consideration to use ginkgo/gomega ?

MaxMustermann2 · 2024-03-20T07:44:23Z

@MaxMustermann2 this is not the case for current implementation, every withdrawal must be processed by Exocore network so client chain contracts can not unlock the asset and allow for further claim by itself. For undelegation workflow, the call's result of ExocoreGateway could only indicate whether the undelegation is queued to be finished or not. We could change the result's name in contract to eliminate misunderstanding

I see; this was a mistake in my understanding of the Solidity implementation.

User undelegates and a message is sent to Exocore chain. A response is returned indicating the success or failure of the undelegation request on the Exocore chain. This response results in an event being emitted on the client chain and nothing else.
When the undelegation matures, the undelegation is marked on Exocore chain as ready for withdrawal.
The user then sends a withdraw transaction on the client chain, which is forwarded to the Exocore chain. The Exocore chain returns the amount that can be withdrawn.
This amount is updated in the Vault contract, and then user needs to withdraw from the Vault.

Thank you for the clarification.

TimmyExogenous · 2024-03-20T07:58:06Z

ginkgo used across all _test file, but it seems only used for evmos/inflation module's integration test, all files imported ginkgo here actually not use it(no any Describe or other ginkgo test usage). Is there any consideration to use ginkgo/gomega ?

The functions RegisterFailHandler and RunSpecs will use them.

leonz789 · 2024-03-20T08:05:47Z

ginkgo used across all _test file, but it seems only used for evmos/inflation module's integration test, all files imported ginkgo here actually not use it(no any Describe or other ginkgo test usage). Is there any consideration to use ginkgo/gomega ?

The functions RegisterFailHandler and RunSpecs will use them.

yes, but those two also meaningless, there's nothing for RunSpecs to run

mikebraver · 2024-03-20T10:04:03Z

proto/exocore/assets/v1/tx.proto

+
+// MsgUpdateParamsResponse defines the response structure for executing a
+// MsgUpdateParams message.
+message MsgUpdateParamsResponse {}


Why didn't add fields in response message?

No data needs to be returned, so an empty struct is defined as the response, with reference to cosmos-sdk.

TimmyExogenous · 2024-03-20T13:06:04Z

ginkgo used across all _test file, but it seems only used for evmos/inflation module's integration test, all files imported ginkgo here actually not use it(no any Describe or other ginkgo test usage). Is there any consideration to use ginkgo/gomega ?

The functions RegisterFailHandler and RunSpecs will use them.

yes, but those two also meaningless, there's nothing for RunSpecs to run

I haven't thoroughly researched it. It might be used to collect the results when running the tests. The output log will be different when I run the tests with it. It's fine to remove it if it doesn't influence the test process.

leonz789 · 2024-03-20T15:51:20Z

ginkgo used across all _test file, but it seems only used for evmos/inflation module's integration test, all files imported ginkgo here actually not use it(no any Describe or other ginkgo test usage). Is there any consideration to use ginkgo/gomega ?

The functions RegisterFailHandler and RunSpecs will use them.

yes, but those two also meaningless, there's nothing for RunSpecs to run

I haven't thoroughly researched it. It might be used to collect the results when running the tests. The output log will be different when I run the tests with it. It's fine to remove it if it doesn't influence the test process.

for the output log print by ginkgo, you can see all the statistics is 0 (passed, failed, skipped), since there's no actual test case related to it

MaxMustermann2 and others added 30 commits February 8, 2024 17:08

refactor the non-compliant code

4949df5

fix the bug caused by rebase merge remove license comments add some code comments change precompile contract solidity version

add more info for error log

20d9e3d

add operator module

f6ccc82

update avs_operator_assets_state

8683574

add opt-out function

abf43f1

update opt-out function

ed545b3

update slash function

7b198d9

update slash function

54dfa8a

finish slash function

60d64b5

update abci.go in operator module

efe31fe

finish abci.go in operator module

6de7a1d

update abci.go in the delegation module

c35bca7

solve the problems identified by code review

31d7b7b

fix some bugs of operator module

8ea56b5

resolve the problems of code style

b2c9d1a

remove the redundant code from the abci.go file in the operator module

e824f47

change file name in operator module

57fcb7a

test getting historical operator info

5c98ed3

add ContextForHistoricalState to retrieve historical state

502b370

fix the testing bug for retrieving histrical operator info

a02457b

update for retrieving historical state

a149345

fix test bug for delegation

f3a6f00

add unit test for operator module

4364b36

fix the problems after rebasing to dogfood

5fbedd0

copy the code from operator_consent to integrate with the dogfood mod…

7168331

…ule.

correct the dependency in x/operator/keeper/utils_test.go

3fd0dbf

delete snapshot and refine some code about operator module

3da8cba

add unit test for slash in operator module

3832849

Test to see if the workflow for checking the PR will be activated.

7d8d25b

rlan35 reviewed Mar 12, 2024

View reviewed changes

TimmyExogenous added 2 commits March 13, 2024 14:23

mv the function related to setting ExocoreLzAppAddress to assets modu…

fa2a36a

…le and delete redudant code rename chainId to chainID in dogfood module rename restakingKeeper to assetsKeeper

fix the proto-lint error

032c5eb

MaxMustermann2 reviewed Mar 13, 2024

View reviewed changes

TimmyExogenous added 3 commits March 14, 2024 10:21

refine the unconvitional code identified by code review in the procom…

03b5c9e

…pile

add code comments for GetAvsSupportedAssets

3c08c5e

Merge branch 'develop' into copy-app-chain

361d0fa

MaxMustermann2 reviewed Mar 15, 2024

View reviewed changes

precompiles/delegation/error.go Show resolved Hide resolved

x/operator/types/expected_keepers.go Outdated Show resolved Hide resolved

MaxMustermann2 reviewed Mar 16, 2024

View reviewed changes

MaxMustermann2 reviewed Mar 17, 2024

View reviewed changes

TimmyExogenous added 2 commits March 18, 2024 12:02

resolve the missed issues identified by code review, such as adding s…

ddaf1a2

…ome comments and refine the naming

fix the prtobuf lint error

e8f6c8c

TimmyExogenous force-pushed the copy-app-chain branch from 8b77813 to e8f6c8c Compare March 18, 2024 06:50

MaxMustermann2 approved these changes Mar 19, 2024

View reviewed changes

remove the unnecessary code related to operator from the delegation p…

4596ca6

…roto file

rlan35 approved these changes Mar 20, 2024

View reviewed changes

mikebraver reviewed Mar 20, 2024

View reviewed changes

mikebraver approved these changes Mar 21, 2024

View reviewed changes

mikebraver merged commit 658364d into develop Mar 21, 2024
20 checks passed

MaxMustermann2 added a commit to MaxMustermann2/exocore that referenced this pull request Mar 21, 2024

Merge imua-xyz#12 into bootstrap-local

0994129

MaxMustermann2 added a commit to MaxMustermann2/exocore that referenced this pull request Mar 21, 2024

Merge imua-xyz#12 into bootstrap-local

659c85f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Resubmit the operator module #12

[feat] Resubmit the operator module #12

TimmyExogenous commented Mar 8, 2024

TimmyExogenous commented Mar 12, 2024

TimmyExogenous commented Mar 13, 2024

MaxMustermann2 left a comment

MaxMustermann2 Mar 16, 2024 •

edited

Loading

TimmyExogenous Mar 18, 2024

MaxMustermann2 Mar 17, 2024

TimmyExogenous Mar 18, 2024

MaxMustermann2 Mar 19, 2024

TimmyExogenous Mar 20, 2024

TimmyExogenous Mar 20, 2024

MaxMustermann2 Mar 20, 2024

MaxMustermann2 Mar 20, 2024

MaxMustermann2 left a comment

MaxMustermann2 Mar 19, 2024

MaxMustermann2 Mar 19, 2024

MaxMustermann2 Mar 19, 2024

MaxMustermann2 Mar 19, 2024

adu-web3 commented Mar 20, 2024

leonz789 commented Mar 20, 2024

MaxMustermann2 commented Mar 20, 2024

TimmyExogenous commented Mar 20, 2024

leonz789 commented Mar 20, 2024 •

edited

Loading

mikebraver Mar 20, 2024

TimmyExogenous Mar 20, 2024

TimmyExogenous commented Mar 20, 2024 •

edited

Loading

leonz789 commented Mar 20, 2024

[feat] Resubmit the operator module #12

[feat] Resubmit the operator module #12

Conversation

TimmyExogenous commented Mar 8, 2024

Description

Todo

TimmyExogenous commented Mar 12, 2024

TimmyExogenous commented Mar 13, 2024

MaxMustermann2 left a comment

Choose a reason for hiding this comment

MaxMustermann2 Mar 16, 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

MaxMustermann2 left a 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

adu-web3 commented Mar 20, 2024

leonz789 commented Mar 20, 2024

MaxMustermann2 commented Mar 20, 2024

TimmyExogenous commented Mar 20, 2024

leonz789 commented Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimmyExogenous commented Mar 20, 2024 • edited Loading

leonz789 commented Mar 20, 2024

MaxMustermann2 Mar 16, 2024 •

edited

Loading

leonz789 commented Mar 20, 2024 •

edited

Loading

TimmyExogenous commented Mar 20, 2024 •

edited

Loading