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

feat: staking precompile delegation and stakingpool queries #1356

Draft
wants to merge 4 commits into
base: evm
Choose a base branch
from

Conversation

kyriediculous
Copy link

Describe your changes and provide context

Testing performed to validate your change

@philipsu522
Copy link
Contributor

@kyriediculous this looks great, do you mind merging?

@kyriediculous
Copy link
Author

kyriediculous commented Mar 1, 2024

@philipsu522 I'm actually working on implementing a more final specification.

See this hackMD doc https://hackmd.io/EcPRzGcPR8OuAxr-48wkRA

My only hurdle at the moment is that the current cosmos-sdk version used by Sei doesn't have the latest protocol buffer messages for staking. So I'll have to use a later version, but this shouldn't be a breaking change.

E.g. the version used in Sei doesn't have all these fields yet in the protobufs: https://github.com/cosmos/cosmos-sdk/blob/a79f932ecf8ef562928fa72989e53ae22e251d7f/proto/cosmos/staking/v1beta1/staking.proto#L239

Hoping to maybe do it this weekend finally ! I didn't have much time last weekend.

@kyriediculous
Copy link
Author

@philipsu522

I added some more undelegation precompiles and the PR is now feature ready for us at Tenderize.

Happy to add a few more generic functions too (e.g. to fetch Validator).

One caveat is that we did have to upgrade sei-cosmos protobuf messages : sei-protocol/sei-cosmos@evm...kyriediculous:sei-cosmos:evm

go.mod Outdated
@@ -8,7 +8,7 @@ require (
github.com/CosmWasm/wasmvm v1.0.1
github.com/armon/go-metrics v0.4.1
github.com/btcsuite/btcd v0.22.1
github.com/cosmos/cosmos-sdk v0.45.10
github.com/cosmos/cosmos-sdk v0.47.10
Copy link
Author

Choose a reason for hiding this comment

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

Should be reversed since we actually meant to upgrade sei-cosmos, not cosmos-sdk

@philipsu522
Copy link
Contributor

@philipsu522

I added some more undelegation precompiles and the PR is now feature ready for us at Tenderize.

Happy to add a few more generic functions too (e.g. to fetch Validator).

One caveat is that we did have to upgrade sei-cosmos protobuf messages : sei-protocol/[email protected]:sei-cosmos:evm

hey @kyriediculous awesome, looks good. One nit thing, for sei-protocol/sei-cosmos@evm...kyriediculous:sei-cosmos:evm our base branch is actually seiv2 - do you think you can rebase your changes (or cherry-pick) on that? we can then get it in.
I've also asked for a 2nd pair of eyes on the labs side

@philipsu522 philipsu522 requested review from codchen and Kbhat1 March 26, 2024 16:47
@kyriediculous
Copy link
Author

@philipsu522
I added some more undelegation precompiles and the PR is now feature ready for us at Tenderize.
Happy to add a few more generic functions too (e.g. to fetch Validator).
One caveat is that we did have to upgrade sei-cosmos protobuf messages : sei-protocol/[email protected]:sei-cosmos:evm

hey @kyriediculous awesome, looks good. One nit thing, for sei-protocol/[email protected]:sei-cosmos:evm our base branch is actually seiv2 - do you think you can rebase your changes (or cherry-pick) on that? we can then get it in. I've also asked for a 2nd pair of eyes on the labs side

Thanks ! will do.

One change we made is change the return types from Messages from boolean to useful data.

Do these functions properly throw a REVERT op code on the EVM side when the messages return an error ?

Or is this why there was a boolean parameter in the first place, because an error in the message execution doesnt actually lead to a REVERT opcode and the expectation is for callers to handle the boolean and revert if it returns false ?

}

func (p Precompile) delegate(ctx sdk.Context, method *abi.Method, caller common.Address, args []interface{}) ([]byte, error) {
pcommon.AssertArgsLength(args, 2)
delegator := p.evmKeeper.GetSeiAddressOrDefault(ctx, caller)
validatorBech32 := args[0].(string)
validatorBech32 := p.evmKeeper.GetSeiAddressOrDefault(ctx, args[0].(common.Address)).String()
Copy link
Collaborator

Choose a reason for hiding this comment

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

validator address is different from normal account address and doesn't have associated EVM address, so we can't change this line

Copy link
Author

Choose a reason for hiding this comment

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

What's the length of these ?

I changed these do address mainly because they were previously string which in solidity is really a dynamic bytes array.

So if the validator addresses are 32 Bytes or shorter we can use a normal value type like bytes32.

Copy link
Author

Choose a reason for hiding this comment

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

kind bump @codchen

}

func (p Precompile) redelegate(ctx sdk.Context, method *abi.Method, caller common.Address, args []interface{}) ([]byte, error) {
pcommon.AssertArgsLength(args, 3)
delegator := p.evmKeeper.GetSeiAddressOrDefault(ctx, caller)
srcValidatorBech32 := args[0].(string)
dstValidatorBech32 := args[1].(string)
srcValidatorBech32 := p.evmKeeper.GetSeiAddressOrDefault(ctx, args[0].(common.Address)).String()
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@nikbhintade
Copy link

nikbhintade commented Jul 15, 2024

Hi @codchen @philipsu522 @Kbhat1 @kyriediculous , I want to know if there is plan to integrate this? As I am working on liquid staking on Sei EVM this would be a lot of help.

If this is integrated into next releases that would be helpful too.

@kyriediculous
Copy link
Author

Hi @codchen @philipsu522 @Kbhat1 @kyriediculous , I want to know if there is plan to integrate this? As I am working on liquid staking on Sei EVM this would be a lot of help.

If this is integrated into next releases that would be helpful too.

Yes we'd still love to :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants