-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor: remove cyclic authz dependencies #16509
Conversation
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.
I think this is pushing the problem further, right? IIRC the end goal is to have modules not import the SDK anymore (right?), so moving AcceptResponse
to types/authz
, solves one issue but creates another one for later. I think it is as well better if the types are closer to where they are used.
Doesn't it make more sense to remove the cyclic dependencies by moving the authorization definitions to authz directly? (However, that is wayyy more breaking).
However, if we go this way, I left one comment.
) | ||
|
||
// TODO: Revisit this once we have propoer gas fee framework. | ||
// Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072 | ||
const gasCostPerIteration = uint64(10) | ||
|
||
var _ authz.Authorization = &StakeAuthorization{} |
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.
This is quite handy, if we go this way, cannot we move the interface in types/authz
?
Codec (x/bank/types/codec.go
) still requires the interface registration anyway, so it does not really remove the cyclic dep as of now.
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.
Codec (x/bank/types/codec.go) still requires the interface registration anyway, so it does not really remove the cyclic dep as of now.
Oops, I meant to move those registrations to authz. Will do that now.
This is quite handy, if we go this way, cannot we move the interface in types/authz?
This is handy while writing, but obviates one of the best features of how interfaces are implemented in go's type system. A type can implement an interface's behavior without forming an explicit relationship on the package in which it is specified.
I think so, but it's not totally clear how that will look yet, or how we'll get there. Given this I think it's good take incremental steps. I consider removing a cyclic dependency by pushing the common dependency down an incremental improvement. We could also consider moving it If we do ever eventually make the transition away from gogo types then implementing an interface method on a generated proto type will not be possible, so we'll need a different design for this authz pattern. I think that will happen eventually, but given that's not in writing either, I consider this an acceptable improvement.
I'm not sure what you're proposing here, the Authorization interface is already in authz. |
Yeah, never mind that, I didn't think we could just move the type registration 😅 |
Description
Closes: #16463
This takes approach 1. from the above issue:
AcceptResponse
down into sdk/types, and changeUpdated
field type toproto.Message
. Refactor usages.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