-
Notifications
You must be signed in to change notification settings - Fork 1
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: restaking module keeper and msg server #31
Conversation
df6ef4b
to
60b17a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Just have some change requests.
Also, can't we just use one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions :)
// GetAllPoolDelegations returns all the pool delegations | ||
func (k *Keeper) GetAllPoolDelegations(ctx sdk.Context) []types.PoolDelegation { | ||
var delegations []types.PoolDelegation | ||
k.IterateAllPoolDelegations(ctx, func(delegation types.PoolDelegation) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this could be implemented with the same way below(GetAllDelegatorPoolDelegations
) instead of calling callback and always returning false.
x/restaking/keeper/restaking.go
Outdated
if err != nil { | ||
return newShares, err | ||
} | ||
|
||
return newShares, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil { | |
return newShares, err | |
} | |
return newShares, nil | |
return newShares, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a personal preference, but I like best returning the error explicitly inside an if
instead of using the last return
statement.
This because if we want, we could also write
return newShares, hooks.AfterDelegationModified(ctx, receiver.GetID(), delegator)
And it would be the same. But this makes it harder to read the code in my opinion. Then we might have the question: how far should we go in choosing between "less code as possible" vs "more code to be more readable"?
In this fight I always choose "more code but easier to read", just to be consistent. This is why I always prefer things like
if err != nil {
return nil, err
}
rather than
return newShares, hooks.AfterDelegationModified(ctx, receiver.GetID(), delegator)
Even though they are identical. Just some lines more won't be an issue performance-wise (also because the compiler will take care of optimizing them). It's just about making it easier to read for other developers and have a consistent code style in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR implements the write operations of the
x/restaking
module. In particular, it contains theKeeper
andMsgServer
methods to execute the various restaking-related messages.All queries and read operations are not present inside this PR, neither are the module registration inside the
App
, nor clients implementations. All of this will be implemented with other PRs in the future. This guarantees that the files changed here are less and easier to review.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change