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

Remove DelegatorAddress from Msg CreateValidator #4595

Closed
Guy1m0 opened this issue Jun 20, 2019 · 5 comments · Fixed by #14567
Closed

Remove DelegatorAddress from Msg CreateValidator #4595

Guy1m0 opened this issue Jun 20, 2019 · 5 comments · Fixed by #14567

Comments

@Guy1m0
Copy link

Guy1m0 commented Jun 20, 2019

For the type MsgCreateValidator, it has the following logic in ValidateBasic

2371561020490_ pic

But when calls its GetSigners(), it checks again if the two address, ValidatorAddress and DelegatorAddress, are equal.

2391561020540_ pic

Therefore, I think the later if-condition might be redundant

@alexanderbez
Copy link
Contributor

I don't think this is quite right. See my response here: #4638 (review)

@rigelrozanski
Copy link
Contributor

It looks as though there is a bit of an inconsistency in the staking logic here.

  • The first check prevents different delegator and validator addresses from being used
  • The second check redundantly checks if the addresses are different (which they can't be due to the first check) - and adds multiple addresses to get signers if so.

This looks as though it is leftover code from the idea of "create-validator-on-behalf-of" logic. I recommend we move the DelegatorAddress from the message structure as @alexanderbez suggested in the PR review

@rigelrozanski rigelrozanski changed the title Redundant if-condition in x/staking/types/msg.go Remove DelegatorAddress from Msg CreateValidator Jun 28, 2019
@alexanderbez
Copy link
Contributor

Do you mind amending the PR @Guy1m0?

@Guy1m0
Copy link
Author

Guy1m0 commented Jun 28, 2019

Feel free. And I think moving the DelegatorAddress is more reasonable.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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