Integrate Packet Forward Middleware #3911

nicolaslara · 2023-01-03T13:28:49Z

What is the purpose of the change

Integrate the packet forward middleware

Brief Changelog

Integrated packet forward middleware.
Should this be in the changelog?

Testing and Verifying

All existing tests that use IBC pass (especially E2E)
Should we add an custom E2E test for PFM?

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes )
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
How is the feature or change documented? (not applicable )

github-actions · 2023-01-19T00:01:57Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

ValarDragon · 2023-01-27T06:45:56Z

app/keepers/keepers.go

+		appKeepers.GetSubspace(packetforwardtypes.ModuleName),
+		appKeepers.TransferKeeper,
+		appKeepers.IBCKeeper.ChannelKeeper,
+		appKeepers.DistrKeeper,


Uhh, why does it need this? Community pool?

yeah, the fee can be configured via a param as a percentage of the send and it funds the community pool.

Ideally this should go to stakers IMO, but the fee defaults to 0, so we can change that later

RIP, this is bad module design imo, should be a middleware responsibility. (Fee at the base transport layer bad design)

Can you please expand on this @ValarDragon ? Would love to understand your perspective so we can improve it. I'm not sure what you mean, as this is the keeper for the IBC middleware.

I don't see why your transport layer for packet sending should have bips of fee bundled in at default. This is equivalent in design practice to saying our API for bank sends should take in a fee parameter as well, and have bips of all sends be captured.

You could in principle want this, but this can be pushed to wrappers of the main object, rather than being the main thing you import. (I'd prefer no capability of this in what we import, than having such a parameter forced)

(NOTE: This is very minor on the overall work going into this. Awesome job on the module! And I think my first comment "bad module design imo" was too harsh for the severity of opinion actually held)

tests/e2e/e2e_test.go

nicolaslara · 2023-01-30T11:55:13Z

@ValarDragon Updated to the newer version of PFM which solves the json encoding issue we discussed. This should be ready for re-review/merge

ValarDragon · 2023-02-06T17:23:25Z

tests/e2e/e2e_test.go

+		count := response["count"].(float64)
+		// check if denom contains "uosmo"
+		return err == nil && count == 0


We need to handle this TODO right? IMO we should ensure exact osmo amount received, in order to be certain that our default post-upgrade parameter has no fee taken.

I think that comment is a leftover from previous code. The balance check is done above (line419) but it's not checking the denom, just the amount. I'll add the denom check.

ValarDragon · 2023-02-07T03:52:34Z

Needs changelog pre-merge

p0mvn · 2023-02-07T18:10:08Z

tests/e2e/e2e_test.go

+		return false
+	},
+		1*time.Minute,
+		10*time.Millisecond,


I think that CI might be flaky and blocking requests due to retrying this too frequently. I've run into similar issues before where AFAIR increasing this number helped.

I looked into the node logs: https://github.com/osmosis-labs/osmosis/suites/10828110105/artifacts/545247532 and they seem okay. So I'm guessing it might be some setup problem

Tried increasing it and it didn't help, but somehow it works (locally) after I added the Require() you suggested

tests/e2e/e2e_test.go

Co-authored-by: Roman <[email protected]>

* added initial packet forward middleware integration * gofumpt * updated go.mod * correct value * updating to match upstream * added params * gofumpt * added PFM to upgrades * go imports * added e2e test for PFM+wasm hoks * refactor * refactored for reusability and added comments * updated to latest version that allows native json * removed unnecessary param * reordering imports after merge * added denom check * added changelog * Update tests/e2e/e2e_test.go Co-authored-by: Roman <[email protected]> * requiring the txs to succeed * fixed e2e tests --------- Co-authored-by: Roman <[email protected]>

* added initial packet forward middleware integration * gofumpt * updated go.mod * correct value * updating to match upstream * added params * gofumpt * added PFM to upgrades * go imports * added e2e test for PFM+wasm hoks * refactor * refactored for reusability and added comments * updated to latest version that allows native json * removed unnecessary param * reordering imports after merge * added denom check * added changelog * Update tests/e2e/e2e_test.go Co-authored-by: Roman <[email protected]> * requiring the txs to succeed * fixed e2e tests --------- Co-authored-by: Roman <[email protected]> (cherry picked from commit 9f14694) # Conflicts: # app/upgrades/v15/constants.go # tests/e2e/e2e_test.go

* Integrate Packet Forward Middleware (#3911) * added initial packet forward middleware integration * gofumpt * updated go.mod * correct value * updating to match upstream * added params * gofumpt * added PFM to upgrades * go imports * added e2e test for PFM+wasm hoks * refactor * refactored for reusability and added comments * updated to latest version that allows native json * removed unnecessary param * reordering imports after merge * added denom check * added changelog * Update tests/e2e/e2e_test.go Co-authored-by: Roman <[email protected]> * requiring the txs to succeed * fixed e2e tests --------- Co-authored-by: Roman <[email protected]> * removed CL test --------- Co-authored-by: Roman <[email protected]>

added initial packet forward middleware integration

89b8b23

github-actions bot added the C:app-wiring Changes to the app folder label Jan 3, 2023

nicolaslara added the V:state/breaking State machine breaking PR label Jan 3, 2023

nicolaslara added 4 commits January 3, 2023 14:40

gofumpt

0820107

Merge branch 'main' into nicolas/packet-forward-middleware

b987095

updated go.mod

aa7dd10

correct value

f084a4b

github-actions bot added the Stale label Jan 19, 2023

Merge branch 'main' into nicolas/packet-forward-middleware

b2654a1

nicolaslara removed the Stale label Jan 23, 2023

nicolaslara added 6 commits January 23, 2023 15:51

updating to match upstream

9d87a60

added params

58859b4

gofumpt

85a936f

Merge branch 'main' into nicolas/packet-forward-middleware

bf161a3

added PFM to upgrades

0b6df24

go imports

0e67297

nicolaslara marked this pull request as ready for review January 24, 2023 10:31

added e2e test for PFM+wasm hoks

19f81c4

ValarDragon reviewed Jan 27, 2023

View reviewed changes

tests/e2e/e2e_test.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jan 27, 2023

View reviewed changes

tests/e2e/e2e_test.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jan 27, 2023

View reviewed changes

tests/e2e/e2e_test.go Show resolved Hide resolved

nicolaslara added 5 commits January 27, 2023 10:37

refactor

dd322be

refactored for reusability and added comments

a4f7082

Merge branch 'main' into nicolas/packet-forward-middleware

03b1d09

updated to latest version that allows native json

293a16c

removed unnecessary param

fea6d54

Merge branch 'main' into nicolas/packet-forward-middleware

9168bb8

ValarDragon reviewed Feb 6, 2023

View reviewed changes

nicolaslara added 4 commits February 7, 2023 13:31

Merge branch 'main' into nicolas/packet-forward-middleware

6542dc3

reordering imports after merge

a014a31

added denom check

8208bfc

added changelog

66c2ccc

p0mvn reviewed Feb 7, 2023

View reviewed changes

tests/e2e/e2e_test.go Outdated Show resolved Hide resolved

nicolaslara and others added 4 commits February 8, 2023 09:37

Update tests/e2e/e2e_test.go

9dbf94d

Co-authored-by: Roman <[email protected]>

requiring the txs to succeed

ef52182

fixed e2e tests

080b7e6

Merge branch 'main' into nicolas/packet-forward-middleware

8b308ac

nicolaslara merged commit 9f14694 into main Feb 8, 2023

nicolaslara deleted the nicolas/packet-forward-middleware branch February 8, 2023 10:01

nicolaslara mentioned this pull request Feb 9, 2023

backport: Integrate Packet Forward Middleware #4281

Merged

nicolaslara added V:state/compatible/backport State machine compatible PR, should be backported and removed V:state/breaking State machine breaking PR V:state/compatible/backport State machine compatible PR, should be backported labels Feb 10, 2023

nicolaslara self-assigned this Feb 10, 2023

ValarDragon added A:backport/v15.x backport patches to v15.x branch V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported labels Feb 10, 2023

mergify bot mentioned this pull request Feb 10, 2023

Integrate Packet Forward Middleware (backport #3911) #4289

Closed

agouin mentioned this pull request May 8, 2023

Migrate PFM to ibc-apps repo strangelove-ventures/packet-forward-middleware#79

Closed

github-actions bot mentioned this pull request May 15, 2024

Broken Links Detected #8269

Closed

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Packet Forward Middleware #3911

Integrate Packet Forward Middleware #3911

nicolaslara commented Jan 3, 2023 •

edited

Loading

github-actions bot commented Jan 19, 2023

ValarDragon Jan 27, 2023

nicolaslara Jan 27, 2023

ValarDragon Jan 29, 2023 •

edited

Loading

agouin Jan 30, 2023

ValarDragon Feb 1, 2023

ValarDragon Feb 1, 2023

nicolaslara commented Jan 30, 2023

ValarDragon Feb 6, 2023 •

edited

Loading

nicolaslara Feb 7, 2023

ValarDragon commented Feb 7, 2023

p0mvn Feb 7, 2023

nicolaslara Feb 8, 2023 •

edited

Loading

Integrate Packet Forward Middleware #3911

Integrate Packet Forward Middleware #3911

Conversation

nicolaslara commented Jan 3, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

github-actions bot commented Jan 19, 2023

ValarDragon Jan 27, 2023

Choose a reason for hiding this comment

nicolaslara Jan 27, 2023

Choose a reason for hiding this comment

ValarDragon Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

agouin Jan 30, 2023

Choose a reason for hiding this comment

ValarDragon Feb 1, 2023

Choose a reason for hiding this comment

ValarDragon Feb 1, 2023

Choose a reason for hiding this comment

nicolaslara commented Jan 30, 2023

ValarDragon Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

nicolaslara Feb 7, 2023

Choose a reason for hiding this comment

ValarDragon commented Feb 7, 2023

p0mvn Feb 7, 2023

Choose a reason for hiding this comment

nicolaslara Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

nicolaslara commented Jan 3, 2023 •

edited

Loading

ValarDragon Jan 29, 2023 •

edited

Loading

ValarDragon Feb 6, 2023 •

edited

Loading

nicolaslara Feb 8, 2023 •

edited

Loading