ExocoreNetwork / exocore

5 stars 9 forks source link

[feat] Resubmit the operator module #12

Closed TimmyExogenous closed 3 months ago

TimmyExogenous commented 4 months ago

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

TimmyExogenous commented 4 months ago

The failing "Protobuf / break-check" workflow can be ignored. It reminds us that some protobuf files have been changed. But It's okay to change them during the early stages of rapid iteration and development. This check won't fail after It's been merged.

TimmyExogenous commented 4 months ago

All problems identified by code review have been resolved.

TimmyExogenous commented 3 months ago

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

adu-web3 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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.

Thank you for the clarification.

TimmyExogenous commented 3 months ago

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 commented 3 months ago

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

TimmyExogenous commented 3 months ago

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 commented 3 months ago

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