[FEAT] ICS20-V2 Path Forwarding #6693

chatton · 2024-06-25T08:05:21Z

Description

Path forwarding feature for ICS20-V2 🚀

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

* chore: adding proto files for ics20-v2 * chore: add newline

* add CurrentVersion, EscrowVersion, update tests * update hardcoded transfer channel version from interchaintest

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

…ods (#6126)

…shalling/conversion (#6226) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * add CurrentVersion, EscrowVersion, update tests * pr review * fix e2e tests * pr review * update e2e test version * linter * update hardcoded transfer channel version from interchaintest * update hardcoded transfer channel version from interchaintest * wip packet unmarshaller * wip * wip testing * update test * linter * rm unnecessary version changes * rm unnecessary artifacts * update callbacks test * update comment * pr review * rename getMultiDenomFungibleTokenPacketData to unmarshalPacketDataBytesToICS20V2 --------- Co-authored-by: chatton <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>

…allbacks (#6175) * add SupportedVersions array for different ics20 versions, add version checking on channel handshake application callbacks * add tests * update pr review * pr review * last few pr review nits * linter * add version counter proposing * fix missing app versino * update code + tests to return our proposed version if counterparty version is invalid * remove if statement * address review comments: return ics20-2 if counterparty version is not supported --------- Co-authored-by: Carlos Rodriguez <[email protected]>

…transfers (#6252) * add transfer authz code + tests * linter * address review comments --------- Co-authored-by: Carlos Rodriguez <[email protected]>

…ks (#6189) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * pr review * linter * rm unused function: linter * wip pr feedback * fix test * pr review * lintttttt * wip: backwards compatibility for transfer rpc * implement changes for ics20-v2 packet data for onRecvPacket, onAcknowledgePacket and onTimeoutPacket * fix callbacks tests * lint --------- Co-authored-by: chatton <[email protected]> Co-authored-by: Charly <[email protected]>

Co-authored-by: Damian Nolan <[email protected]>

AdityaSripal

Reviewed the non-test files and acking for merge.

I do have some nits on structuring. In general, I found the use of helper functions too extensive, to the point where it hurt understandability more than it helped. Left comments where relevant. This can be discussed and fixed in later PRs

Acking to unblock since i mainly wanted to review the overall logic in key places. I can review test files as well another day if folks want me to

modules/apps/callbacks/ibc_middleware_test.go

modules/apps/transfer/client/cli/tx.go

modules/apps/transfer/keeper/forwarding.go

modules/apps/transfer/keeper/relay.go

modules/apps/transfer/types/forwarding.go

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

colin-axner

Woo!! Reviewed the core logic and left any relevant notes. I skimmed over the tests, but largely relying on the fact that we are checking code cov and have already reviewed all the files changed. Happy to see this merged and followup issues addressed after the fact

modules/apps/transfer/keeper/relay.go

damiannolan

LGTM! Thanks everyone for their work on this feature! ❤️

modules/apps/transfer/keeper/msg_server.go

modules/apps/transfer/keeper/relay.go

damiannolan · 2024-06-27T14:40:07Z

modules/apps/transfer/types/keys.go


 	ParamsKey = "params"
+
+	KeyPacketForwardPrefix = "forwardedPacket"


Let's group them together in a follow up PR

modules/apps/transfer/types/transfer_authorization.go

modules/apps/transfer/types/forwarding.go

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

chatton

Can't leave an approval since I created the PR but I can leave a 🟢 .

DimitrisJim

absolutely amazing job everyone!

Going to leave for tomorrow's early bird to merge, whomever that might be

crodriguezvega · 2024-06-28T07:17:49Z

I am looking at the upgrade tests that fail to see if I can do a quick fix before merging.

sonarcloud · 2024-06-28T07:35:42Z

Quality Gate passed for 'ibc-go'

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

chatton and others added 30 commits April 8, 2024 13:00

Adding proto files for ics20-v2 (#6110)

c07bca9

* chore: adding proto files for ics20-v2 * chore: add newline

update amount -> string (#6120)

e66bd89

Update MsgTransfer to accept sdk.Coins instead of sdk.Coin (#6113)

034f472

fix: allow base denom with trailing slash (#6148)

4cc6a85

imp: add CurrentVersion, EscrowVersion (#6160)

71f830c

* add CurrentVersion, EscrowVersion, update tests * update hardcoded transfer channel version from interchaintest

chore: add function for converting packet data from v1 to v3 (#6116)

28ff9b6

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

chore: implement required FungibleTokenPacketData v3 interface meth…

4e55137

…ods (#6126)

imp: update transfer authz implementation to account for multi denom …

9bbfa1a

…transfers (#6252) * add transfer authz code + tests * linter * address review comments --------- Co-authored-by: Carlos Rodriguez <[email protected]>

add v3 packet proto

e04047e

fix protos

c73d5f6

fixes

03cc2e5

test fixes

65f4476

add forwardPath keys and memo check in sendTransfer

db85a29

wip onRecvPacket logic

999ed80

minor fixes

be72180

changes to transfer tx CLI to support multiple denoms

4c333c5

import renaming

0478cb9

onRecv logic completed

37f5b5e

add revertInFlights function

3fd3345

add onAck && onTimeout logic

24e2407

fix interchain accounts test

7858528

basic unit test for path forwarding

55d7bf9

fix test unsuccessful refund from source

1a95262

wip test fix

44e15cb

fix mbt test - need more investigation

334d5a9

revert test fix

7466b19

add assertions

5c46d79

nit: improve documentation for transfer's OnRecv callback.

25c73c4

Co-authored-by: Damian Nolan <[email protected]>

AdityaSripal approved these changes Jun 27, 2024

View reviewed changes

Apply suggestions from code review

baf3fd3

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

colin-axner approved these changes Jun 27, 2024

View reviewed changes

modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved

modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved

Carlos Rodriguez added 3 commits June 27, 2024 16:15

chore: remove unused function

7d2a881

perf: allocate slice to length of packet data tokens

4ea36c2

Merge branch 'main' into feat/ics20-v2-path-forwarding

0a28e9b

DimitrisJim mentioned this pull request Jun 27, 2024

Remove helper function usage during forwarding #6722

Closed

3 tasks

damiannolan approved these changes Jun 27, 2024

View reviewed changes

DimitrisJim mentioned this pull request Jun 27, 2024

Add a NoForwarding helper function to use in tests #6723

Closed

3 tasks

damiannolan mentioned this pull request Jun 27, 2024

Restructure transfer ibc module godocs for forwarding #6724

Closed

damiannolan and others added 2 commits June 27, 2024 16:59

chore(transfer/authz): wrapf unauthorized forwarding hops

7411f2d

lint

199c14a

DimitrisJim reviewed Jun 27, 2024

View reviewed changes

modules/apps/transfer/types/forwarding.go Outdated Show resolved Hide resolved

Carlos Rodriguez and others added 2 commits June 27, 2024 17:03

Update modules/apps/transfer/types/forwarding.go

c7af85a

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

Merge branch 'main' into feat/ics20-v2-path-forwarding

25239a2

crodriguezvega mentioned this pull request Jun 27, 2024

ICS20-V2 Path Forwarding E2E Tests #6578

Closed

8 tasks

chatton commented Jun 27, 2024

View reviewed changes

Preallocate slice but keep len==0 (#6725)

90d57db

DimitrisJim approved these changes Jun 27, 2024

View reviewed changes

Carlos Rodriguez added 2 commits June 27, 2024 20:57

imp: validate allowed forwarding hops

e310e27

test: unwind fails in Transfer rpc

f28e937

Merge branch 'main' into feat/ics20-v2-path-forwarding

50daed0

crodriguezvega added this pull request to the merge queue Jun 28, 2024

Merged via the queue into main with commit 1812138 Jun 28, 2024
131 of 146 checks passed

DimitrisJim mentioned this pull request Jun 28, 2024

nit: prebuild the token array during send transfer #6729

Merged

10 tasks

crodriguezvega deleted the feat/ics20-v2-path-forwarding branch July 9, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] ICS20-V2 Path Forwarding #6693

[FEAT] ICS20-V2 Path Forwarding #6693

chatton commented Jun 25, 2024 •

edited

Loading

AdityaSripal left a comment

colin-axner left a comment

damiannolan left a comment

damiannolan Jun 27, 2024

chatton left a comment

DimitrisJim left a comment •

edited

Loading

crodriguezvega commented Jun 28, 2024

sonarcloud bot commented Jun 28, 2024


		ParamsKey = "params"

		KeyPacketForwardPrefix = "forwardedPacket"

[FEAT] ICS20-V2 Path Forwarding #6693

[FEAT] ICS20-V2 Path Forwarding #6693

Conversation

chatton commented Jun 25, 2024 • edited Loading

Description

AdityaSripal left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jun 27, 2024

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

DimitrisJim left a comment • edited Loading

Choose a reason for hiding this comment

crodriguezvega commented Jun 28, 2024

sonarcloud bot commented Jun 28, 2024

Quality Gate passed for 'ibc-go'

chatton commented Jun 25, 2024 •

edited

Loading

DimitrisJim left a comment •

edited

Loading