-
Notifications
You must be signed in to change notification settings - Fork 352
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
Problem: Missing ICA auth module for enabling interchain accounts #730
Problem: Missing ICA auth module for enabling interchain accounts #730
Conversation
Solution: Added a basic ICA auth module
Codecov Report
@@ Coverage Diff @@
## master #730 +/- ##
==========================================
+ Coverage 19.45% 20.60% +1.14%
==========================================
Files 70 76 +6
Lines 7931 9989 +2058
==========================================
+ Hits 1543 2058 +515
- Misses 5876 7465 +1589
+ Partials 512 466 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
does it need to be a separate module or could the functionality be added to an existing module? I'm thinking that x/chainmain is pretty much empty, but ok to keep it separate too
It does not need to be in a separate module but it's better to keep it separate for clarity and logical separation. |
ok, I'll need to read https://github.com/cosmos/ibc-go/blob/v3.0.0/docs/apps/interchain-accounts/auth-modules.md first |
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'm a bit puzzled about the OnAcknowledgementPacket
section -- is that done in ibc-go?
Yes. For packet acknowledgement, both |
var _ status.Status | ||
var _ = runtime.String | ||
var _ = utilities.NewDoubleArray | ||
var _ = descriptor.ForMessage |
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.
SA1019: descriptor.ForMessage is deprecated: Not all concrete message types satisfy the Message interface. Use MessageDescriptorProto instead. If possible, the calling code should be rewritten to use protobuf reflection instead. See package "google.golang.org/protobuf/reflect/protoreflect" for details.
(at-me in a reply with help
or ignore
)
"io" | ||
"net/http" | ||
|
||
"github.com/golang/protobuf/descriptor" |
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.
SA1019: package github.com/golang/protobuf/descriptor is deprecated: See the "google.golang.org/protobuf/reflect/protoreflect" package for how to obtain an EnumDescriptor or MessageDescriptor in order to programatically interact with the protobuf type system.
(at-me in a reply with help
or ignore
)
"net/http" | ||
|
||
"github.com/golang/protobuf/descriptor" | ||
"github.com/golang/protobuf/proto" |
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.
SA1019: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.
(at-me in a reply with help
or ignore
)
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 probably ok -- the app wiring, upgrade handler etc. will be a separate PR if I understand the plan
@devashishdxt would a different protobuf generator version use non-deprecated packages? |
Maybe if we upgrade to newer version then it won't use deprecated packages. But, I don't think we can stop using these deprecated packages in our code until |
I can see that |
Solution: Added a basic ICA auth module
As described in #719, interchain accounts feature will be divided in three separate PRs. This is the first one which adds a basic ICA auth module.
Ref: https://github.com/cosmos/ibc-go/blob/v3.0.0/docs/apps/interchain-accounts/auth-modules.md