-
Notifications
You must be signed in to change notification settings - Fork 235
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
Feature: replace ibc-hook with middleware #511
Feature: replace ibc-hook with middleware #511
Conversation
Codecov Report
@@ Coverage Diff @@
## main #511 +/- ##
==========================================
+ Coverage 39.63% 39.91% +0.27%
==========================================
Files 31 30 -1
Lines 1607 1596 -11
==========================================
Hits 637 637
+ Misses 924 913 -11
Partials 46 46
|
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.
🎉awesome.
I guess the problem statement here is: Problem: custom ibc-go fork used
? the docs can perhaps have a brief update: https://github.com/crypto-org-chain/cronos/blob/main/docs/architecture/adr-002.md and https://github.com/crypto-org-chain/cronos/blob/main/docs/architecture/adr-004.md that the middleware implementation will override OnRecvPacket
, OnAcknowledgmentPacket
, and OnTimeoutPacket
?
One potential thing to check is that there's a missing test for timeouts etc.: #398 -- we can see where it's at with the QA team... if ready soon, maybe we can merge the missing integration test first and run it against this?
x/cronos/types/interfaces.go
Outdated
@@ -76,3 +79,10 @@ type EvmKeeper interface { | |||
) (sdk.Coins, error) | |||
ChainID() *big.Int | |||
} | |||
|
|||
// ICS4Wrapper defines the expected ICS4Wrapper for middleware | |||
type ICS4Wrapper interface { |
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 seems not used anywhere?
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.
yes this can be removed for now (this is needed if we need to write a "full" middleware)
5ccbf4c
to
ab8996c
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.
LGTM
f100a9c
to
f429bba
Compare
I will merge this PR first as the current integration tests are passing. I will work on the integration tests for ibc timeout/failure in another PR |
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
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)