Aditya's review PR [DO NOT MERGE] #8558

gjermundgaraba · 2025-06-22T21:31:53Z

This PR is just to have a place to collect reviews from PR of main.
The target branch is from just before PFM was added, so all changes after that are included.

Co-authored-by: Gjermund Garaba <[email protected]>

… code (#8417)

Co-authored-by: Gjermund Garaba <[email protected]>

…sage (#8366) Co-authored-by: Tamjid Ahmed <[email protected]> Co-authored-by: Gjermund Garaba <[email protected]>

Co-authored-by: Gjermund Garaba <[email protected]>

Co-authored-by: Tamjid Ahmed <[email protected]> Co-authored-by: Gjermund Garaba <[email protected]>

…dules/light-clients/08-wasm (#8515) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* chore: rm checksums from proto and migrator * chore: update proto files * chore: use undeprecated option in buf lock * docs: add breaking change to changelog * docs: add space * Update CHANGELOG.md --------- Co-authored-by: Gjermund Garaba <[email protected]>

* test: genesis export * Genesis test * test: keeper for pfm * test: pfm keeper -> Done * fix: Lint. Shutup Meg * remove comment * Rename mock transfer * refactor: clean up test code * update deps * fix deps * refactor: rename malleates -> malleate

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gjermund Garaba <[email protected]>

Co-authored-by: Gjermund Garaba <[email protected]>

…dules/light-clients/08-wasm (#8532) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Co-authored-by: Gjermund Garaba <[email protected]>

Co-authored-by: Gjermund Garaba <[email protected]> Co-authored-by: Alex | Interchain Labs <[email protected]>

Co-authored-by: Gjermund Garaba <[email protected]> Co-authored-by: mconcat <[email protected]> Co-authored-by: Aditya <[email protected]> Co-authored-by: tamjidahmed <[email protected]>

Co-authored-by: Tamjid Ahmed <[email protected]>

modules/apps/packet-forward-middleware/keeper/genesis.go

+	for key, value := range state.InFlightPackets {
+		bz := k.cdc.MustMarshal(&value)
+		if err := store.Set([]byte(key), bz); err != nil {
+			panic(err)
+		}
+	}


modules/core/api/router.go

+	for prefix := range rtr.prefixRoutes {
+		// Prevent existing prefix routes from colliding with the new direct route to avoid confusing behavior.
+		if strings.HasPrefix(portID, prefix) {
+			panic(fmt.Errorf("route %s is already matched by registered prefix route: %s", portID, prefix))
+		}
+	}


modules/core/api/router.go

+	for portID := range rtr.routes {
+		if strings.HasPrefix(portID, portIDPrefix) {
+			panic(fmt.Errorf("route prefix %s is a prefix for already registered route: %s", portIDPrefix, portID))
+		}
+	}


* remove restriction in types package * fix acknowledgement, and start keeper package testing * 04-channel tests * add transfer tests * CHANGELOG * lint * address review --------- Co-authored-by: Gjermund Garaba <[email protected]>

codecov · 2025-06-23T12:46:38Z

Codecov Report

Attention: Patch coverage is 49.62457% with 738 lines in your changes missing coverage. Please review.

Project coverage is 57.99%. Comparing base (33d2d18) to head (e54a696).

Files with missing lines	Patch %	Lines
...es/apps/packet-forward-middleware/keeper/keeper.go	52.08%	121 Missing and 17 partials ⚠️
modules/apps/rate-limiting/client/cli/query.go	0.00%	137 Missing ⚠️
e2e/testsuite/testsuite.go	0.00%	112 Missing ⚠️
...s/apps/packet-forward-middleware/ibc_middleware.go	55.42%	98 Missing and 13 partials ⚠️
modules/apps/rate-limiting/ibc_middleware.go	0.00%	56 Missing ⚠️
...packet-forward-middleware/migrations/v3/migrate.go	0.00%	39 Missing ⚠️
modules/apps/callbacks/testing/simapp/app.go	0.00%	31 Missing ⚠️
modules/apps/packet-forward-middleware/module.go	56.81%	16 Missing and 3 partials ⚠️
modules/apps/rate-limiting/client/cli/cli.go	0.00%	17 Missing ⚠️
modules/apps/rate-limiting/keeper/epoch.go	62.50%	10 Missing and 5 partials ⚠️
... and 18 more

Additional details and impacted files

@@                      Coverage Diff                       @@
##           gjermund/tmp-review-branch    #8558      +/-   ##
==============================================================
- Coverage                       67.17%   57.99%   -9.19%     
==============================================================
  Files                             280      319      +39     
  Lines                           19429    22814    +3385     
==============================================================
+ Hits                            13052    13231     +179     
- Misses                           5794     8978    +3184     
- Partials                          583      605      +22

Flag	Coverage Δ
08-wasm	`65.99% <ø> (+21.70%)`	⬆️
e2e	`1.13% <0.00%> (?)`
ibc-go	`63.43% <54.37%> (-5.44%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-authored-by: Tamjid Ahmed <[email protected]>

Co-authored-by: Gjermund Garaba <[email protected]>

AdityaSripal

Very large PR. So I looked at PFM and rate limit to focus on the changes that i wasn't deeply involved in

AdityaSripal · 2025-07-22T09:46:51Z

docs/docs/04-middleware/02-packet-forward-middleware/02-integration.md

+// transfer stack contains (from top to bottom):
+// - Packet Forward Middleware
+// - Transfer
+var transferStack ibcporttypes.IBCModule


Use new stack primitive

AdityaSripal · 2025-07-22T09:57:52Z

modules/apps/packet-forward-middleware/ibc_middleware.go

+		if len(denom.Trace) == 0 {
+			// denom is now unwound back to native denom
+			return denom.Path()
+		}
+		// denom is still IBC denom
+		return denom.IBCDenom()


Suggested change

if len(denom.Trace) == 0 {

// denom is now unwound back to native denom

return denom.Path()

}

// denom is still IBC denom

return denom.IBCDenom()

return denom.IBCDenom()

IBCDenom does this logic internally

AdityaSripal · 2025-07-29T11:30:41Z

modules/apps/packet-forward-middleware/keeper/keeper.go

+			"refund-port-id", inFlightPacket.RefundPortId,
+		)
+
+		return inFlightPacket, fmt.Errorf("giving up on packet on channel (%s) port (%s) after max retries", inFlightPacket.RefundChannelId, inFlightPacket.RefundPortId)


I don't like how this function is designed where it can return an inFlightpacket and an error. Not intuitive

AdityaSripal · 2025-07-29T11:38:33Z

modules/apps/rate-limiting/keeper/epoch.go

+	// If GetHourEpoch returned a zero-value epoch (due to error or missing key),
+	// we cannot proceed with the check.
+	if hourEpoch.Duration == 0 || hourEpoch.EpochStartTime.IsZero() {
+		return false, 0, errorsmod.Wrapf(types.ErrInvalidEpoce, "cannot check hour epoch starting. epoch: %v", hourEpoch)


Misspelled epoch in error

AdityaSripal · 2025-07-29T12:05:23Z

modules/apps/rate-limiting/v2/ibc_middleware.go

+	return im.app.OnAcknowledgementPacket(ctx, sourceClient, destinationClient, sequence, acknowledgement, payload, relayer)
+}
+
+// TODO: Something looks off about this, please review carefully


This looks correct to me

DeshErBojhaa and others added 30 commits May 15, 2025 15:05

feat: Packet forward middleware (#8285)

9ccab29

Co-authored-by: Gjermund Garaba <[email protected]>

chore: add @DeshErBojhaa as codeowner (#8393)

d0dc0f6

chore: group dependabot bumps (#8403)

a6217ab

chore: dependabot update allow all (#8416)

a411706

chore: only allow direct dependencies for dependabot (#8424)

13389ae

chore: fix codecov reports (#8431)

7ef7ea6

chore: make all actions use version instead of hash (#8426)

5534d13

chore: make codecov only comment when coverage drops or uncovered new…

478cf11

… code (#8417)

Update docusaurus.config.js (#8384)

623f9a5

Co-authored-by: Gjermund Garaba <[email protected]>

docs: add deepwiki badge to readme (#8436)

b509a83

chore: bump hermes to latest version (#8432)

6b2c74f

chore: bump proto image and clean up makefile (#8430)

a5ad744

feat: add field in callbacks data for custom calldata (#8353)

7d579b9

Co-authored-by: Gjermund Garaba <[email protected]>

chore: update style guide (#8415)

ab224d1

Co-authored-by: Gjermund Garaba <[email protected]>

chore: experiment with agents file (#8449)

68270a3

chore: lint and pr link test (#8450)

e64769f

chore: enable ci e2e to run from forks (#8452)

4c6b78f

fix: replace deprecated codec.ProtoMarshaler interface with proto.Mes…

db430f5

…sage (#8366) Co-authored-by: Tamjid Ahmed <[email protected]> Co-authored-by: Gjermund Garaba <[email protected]>

imp: separate direct routing from prefix routing in IBCv2 router (#8446)

ed5789b

Co-authored-by: Gjermund Garaba <[email protected]>

chore: bump go to 1.24 (#8451)

3ba1984

chore: fix import order in testing (#8457)

cd88058

chore: GH workflow for running tidy-all on dependabot PRs (#8456)

df86e7d

Co-authored-by: Gjermund Garaba <[email protected]>

chore: update hermes tag to 1.13.1 (#8459)

0e56395

chore: add v8.8.x backporting (#8482)

78b0abe

fix: support amino encoding on MsgRecoverClient (#8448)

de2f70a

Co-authored-by: Gjermund Garaba <[email protected]>

refactor: rm legacySubspace from NewKeeper funcs (#8476)

fbe9583

Co-authored-by: Gjermund Garaba <[email protected]>

docs: add pfm docs (#8474)

ebf3843

Co-authored-by: Tamjid Ahmed <[email protected]> Co-authored-by: Gjermund Garaba <[email protected]>

chore: use find for md discovery and relax link rules (#8488)

1d153e4

chore: lint codec (#8487)

e2b0a75

chore: add callbacks calldata changelog entry (#8489)

f01d44b

dependabot bot and others added 19 commits June 10, 2025 17:32

chore(deps): bump cosmossdk.io/collections from 1.2.1 to 1.3.0 in /mo…

b4c3756

…dules/light-clients/08-wasm (#8515) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

chore: decrement proto version (#8517)

99a847a

chore: bump proto (#8519)

72bfd08

chore(deps): bump golang.org/x/mod from 0.24.0 to 0.25.0 in /e2e (#8513)

b0550d6

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gjermund Garaba <[email protected]>

chore: rm misbehaviour endpoint (#8516)

68e944f

Co-authored-by: Gjermund Garaba <[email protected]>

chore: set codecov to be informational in ci checks (#8527)

46d4b76

chore: Add unit tests for PFM migration (#8525)

53ac3ce

chore: Use internal transfer representation (#8520)

056e026

Co-authored-by: Gjermund Garaba <[email protected]>

test: enable and fix pfm timeout test (#8526)

904047f

chore(deps): bump cosmossdk.io/collections from 1.3.0 to 1.3.1 in /mo…

9e4cf68

…dules/light-clients/08-wasm (#8532) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

feat: channel aliasing with migration (#8473)

b31bf93

Co-authored-by: Gjermund Garaba <[email protected]>

chore: switch to cosmos/interchaintest (#8486)

2d98792

Co-authored-by: Gjermund Garaba <[email protected]> Co-authored-by: Alex | Interchain Labs <[email protected]>

feat: rate limit module (#8268)

4dc2e77

Co-authored-by: Gjermund Garaba <[email protected]> Co-authored-by: mconcat <[email protected]> Co-authored-by: Aditya <[email protected]> Co-authored-by: tamjidahmed <[email protected]>

Update 02-developIBCv2.md (#8548)

941b95c

Co-authored-by: Tamjid Ahmed <[email protected]>

alias changelog (#8551)

f0587cd

chore: update, tighten and modernize linters (#8496)

1dae432

chore: remove crisis module from simapps (#8540)

5f8fd48

gjermundgaraba requested a review from AdityaSripal June 22, 2025 21:31

gjermundgaraba assigned AdityaSripal Jun 22, 2025

github-advanced-security bot found potential problems Jun 22, 2025

View reviewed changes

Multi payload packet (#8545)

38dd181

* remove restriction in types package * fix acknowledgement, and start keeper package testing * 04-channel tests * add transfer tests * CHANGELOG * lint * address review --------- Co-authored-by: Gjermund Garaba <[email protected]>

AdityaSripal and others added 4 commits June 24, 2025 21:21

update simapp to support v2 with transfer (#8560)

16eb140

chore: remove orderedmap (#8555)

4d99a06

Co-authored-by: Tamjid Ahmed <[email protected]>

Middleware Wiring Improvements (#8528)

72ce552

Co-authored-by: Gjermund Garaba <[email protected]>

[IBCGO-10] Rate Limting Overview and Setting Limits (#8576)

e54a696

Co-authored-by: Gjermund Garaba <[email protected]>

AdityaSripal reviewed Jul 29, 2025

View reviewed changes

remove myself from codeowners (#8596)

698cc74

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Aditya's review PR [DO NOT MERGE] #8558

Aditya's review PR [DO NOT MERGE] #8558

gjermundgaraba commented Jun 22, 2025

Uh oh!

Check warning

Check warning

Check warning

codecov bot commented Jun 23, 2025 •

edited

Loading

Uh oh!

AdityaSripal left a comment

Uh oh!

AdityaSripal Jul 22, 2025

Uh oh!

AdityaSripal Jul 22, 2025

Uh oh!

AdityaSripal Jul 29, 2025

Uh oh!

AdityaSripal Jul 29, 2025

Uh oh!

AdityaSripal Jul 29, 2025

Uh oh!

Uh oh!

Aditya's review PR [DO NOT MERGE] #8558

Are you sure you want to change the base?

Aditya's review PR [DO NOT MERGE] #8558

Conversation

gjermundgaraba commented Jun 22, 2025

Uh oh!

Check warning

Check warning

Check warning

codecov bot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AdityaSripal left a comment

Choose a reason for hiding this comment

Uh oh!

AdityaSripal Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

AdityaSripal Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

AdityaSripal Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

AdityaSripal Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

AdityaSripal Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 23, 2025 •

edited

Loading