Ibcmodulev2 transfer application spike #7524

bznein · 2024-10-29T14:00:07Z

Description

This Spike was done by Cian & Nikolas to help inform the IBCModuleV2 interface definition and to get a PoC of what a transfer v2 module looks like and ensures that funigibility between v1 and v2 can be retained.

A few notes about what we did here before anything goes into the feature branch.

We created a new Keeper under transfer/v2. This keeper embeds the existing transfer/v1 keeper, and overrides functions required. E.g. the new OnRecv, Ack, Timeout etc.
Forwarding is not supported in this PoC. This will require further discussion and spec changes.
Some types/functions were made public to be accessible from the new transfer/v2 Keeper. This can be cleaned up / refactored before merging to not need to do this.
A new IBCModule (V2) implementation for transfer was created, and added to the V2 router in our testing simapp.
Events / telemetry currently disgregarded.
The sequence was added to the OnRecv application callback, as this can be required by some applications. I think we probably need to add this to Ack/Timeout as well, but for now have not done so.
We added some basic tests for all the new transfer module at the message server layer. A simple happy path test and some test cases where the transfer application fails.
We added a test which does the following
- Sets up 3 chains
- Sends tokens from A -> B using MsgTransfer (protocol v1)
- Sends same tokens from B -> C using MsgSendPacket (protocol v2)
- Sends tokens back from C -> B using MsgSendPacket (protocol v2)
- Sends tokens from B -> A using MsgTransfer (protocol V1)

This test ensures that fungibility remains and tokens can be sent back and forth using either the v1 or v2 router.

Questions

What does forwarding look like in transfer v2? In v1 we stored the forwarded packets in state, we no longer provide the entire packet to the application callbacks.
Are we happy with the general idea of re-using the transfer keeper and extending/overriding with new logic where appropriate.
If we are going to include the sequence, should we also include the timeout timestamp? Previously we included the full packet in the application callbacks, but now we are providing a subset of that information. Are there use cases where this extra info should be needed? I guess there is nothing stopping an application from including the timestamp in the packet data if required. I think it's probably fine to omit.

closes: #XXXX

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.

…d sequence

…sfer module

…oth v1 and v2

gjermundgaraba · 2024-10-30T13:18:05Z

Are we happy with the general idea of re-using the transfer keeper and extending/overriding with new logic where appropriate.

To me, this depends a lot on how long we expect v1 to live in our codebase. Short-term, this might be OK, I don't love the idea of a v2 depending on v1 and having these convoluted paths that might end up with weird bugs later. For one release, I might be OK :)

I guess there is nothing stopping an application from including the timestamp in the packet data if required. I think it's probably fine to omit.

But the application would not know if the timestamp is the same as in the packet, unless only the application is allowed to construct new packets on all implementations?

AdityaSripal

I looked mainly at the high level interfaces. We want the sequence in ack and timeout as well.

AdityaSripal · 2024-10-31T16:05:05Z

modules/core/api/module.go

@@ -27,6 +27,7 @@ type IBCModule interface {
 		ctx context.Context,
 		sourceChannel string,
 		destinationChannel string,
+		sequence uint64,


This should be added to OnAcknowledgementPacket and OnTimeoutPacket as well.

yep makes sense!

chatton · 2024-11-04T09:33:04Z

Are we happy with the general idea of re-using the transfer keeper and extending/overriding with new logic where appropriate.

To me, this depends a lot on how long we expect v1 to live in our codebase. Short-term, this might be OK, I don't love the idea of a v2 depending on v1 and having these convoluted paths that might end up with weird bugs later. For one release, I might be OK :)

Thinking a bit more about this, I was thinking it might be possible to create some type that is a subset of of the transfer keeper that has all the functionality that is shared (e.g. code regarding denoms/ native vs ibc etc. )

Extracting some interfaces/ functions into a the transfer v1 module, then having (potentially a new go mod for) transfer v2 import from v1 the required components/functions and implement all the new logic in a new keeper while re-using a subset from v1.

Kind of similar to what is happening in this PR, but this is a bit messier just shoving it all in a sub directory.

…fer-application-spike

gjermundgaraba · 2024-11-18T12:51:08Z

For some context: this PR is being split up in such a way that forwarding support will happen in a separate PR.

gjermundgaraba

I think this looks good! And I've tested it e2e with solidity already, so happy path should be working at the very least :)

gjermundgaraba · 2024-11-18T15:15:11Z

modules/apps/transfer/types/packet.go

@@ -210,6 +210,8 @@ func (ftpd FungibleTokenPacketDataV2) HasForwarding() bool {
 // UnmarshalPacketData attempts to unmarshal the provided packet data bytes into a FungibleTokenPacketDataV2.
 // The version of ics20 should be provided and should be either ics20-1 or ics20-2.
 func UnmarshalPacketData(bz []byte, ics20Version string) (FungibleTokenPacketDataV2, error) {
+	// TODO: in transfer ibc module V2, we need to respect he encoding value passed via the payload, some hard coded assumptions about


Do we have an issue created for this?

I actually have the commit ready to be added to this PR (if we want to!) or I can add it in a separate one! I just wanted to go over it w/ Cian before

AdityaSripal

LGTM, i have a couple suggestions but not blocking ack. I'm also a bit confused with how the app encoding logic

AdityaSripal · 2024-11-18T16:18:14Z

modules/apps/transfer/v2/keeper/msg_server_test.go

+			// TODO: note, encoding field currently not respected in the implementation. encoding is determined by the version.
+			// ics20-v1 == json
+			// ics20-v2 == proto
+			payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz)


Shouldn't this be "proto"?

Yes it should! I actually encountered some errors due to this while working on using the encoding! Not worth fixing it now since I have already a commit (which might end up here on in a separate PR) that fixes it

AdityaSripal · 2024-11-18T16:41:43Z

testing/mock/v2/ibc_app.go

+	OnSendPacket            func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, signer sdk.AccAddress) error
+	OnRecvPacket            func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, relayer sdk.AccAddress) channeltypesv2.RecvPacketResult
+	OnTimeoutPacket         func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, relayer sdk.AccAddress) error
+	OnAcknowledgementPacket func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, acknowledgement []byte, relayer sdk.AccAddress) error


This fn signature looks different than the fn outside the testing package

AdityaSripal · 2024-11-18T16:43:53Z

modules/apps/transfer/v2/ibc_module.go

+		return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", signer)
+	}
+
+	data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version)


Ahh because you aren't using the payload.Encoding here

sonarcloud · 2024-11-19T09:07:34Z

Quality Gate passed for 'ibc-go'

Issues
0 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 SonarQube Cloud

chatton and others added 12 commits October 23, 2024 14:25

chore: adding ibc module v2 and on recv implementation with hard code…

7a257c3

…d sequence

chore: adding ack callback

6608b93

chore: adding send packet

3c23128

chore: adding transfer v2 module to app wiring

c7a74a6

chore: adding happy path and basic failure for send and recv for tran…

4f70fc4

…sfer module

chore: adding ack test for transfer

49392cd

chore: fix some linter errors

d3b19c9

chore: adding timeout test for transfer v2

615f956

chore: adding test case which ensures tokens can be transfered over b…

c03b8c9

…oth v1 and v2

chore: full transfer flow from A - B - C - B - A

0e72c17

chore: separated test out into subtests

198996b

chore: add sequence as argument to OnRecvPacket

973f2f7

chore: adding TODOs for next steps

f53d865

AdityaSripal reviewed Nov 1, 2024

View reviewed changes

chatton and others added 12 commits November 4, 2024 12:11

chore: adding transferv2 module to wasm simapp

cec9d52

chore: refactor OnTimeout to accept sequence

8d6f0a8

chore: refactor OnAck to accept sequence

756c52b

chore: switch argument order

20bae59

wip: mid merge with feature branch, build will be broken

16700f9

Merge branch 'feat/ibc-eureka' into cian/issue#7510-ibcmodulev2-trans…

c2f6937

…fer-application-spike

Fix timeoutTimestamp for tests

b2a1514

linter

20afb28

chore: removing duplicate imports in wasm simapp

29f1151

chore: adding channelkeeperv2 query server

a6c801a

Merge branch 'feat/ibc-eureka' into cian/issue#7510-ibcmodulev2-trans…

6d68fb6

…fer-application-spike

register grpc gateway routes

dbf6f2c

gjermundgaraba mentioned this pull request Nov 14, 2024

Use feat/ibc-eureka wasm simapp image cosmos/solidity-ibc-eureka#94

Closed

Merge branch 'feat/ibc-eureka' into cian/issue#7510-ibcmodulev2-trans…

739af65

…fer-application-spike

fix ack structure for v2 transfer tests

d7c9437

bznein marked this pull request as ready for review November 15, 2024 12:18

bznein requested review from damiannolan, DimitrisJim, srdtrk and gjermundgaraba as code owners November 15, 2024 12:18

womensrights linked an issue Nov 15, 2024 that may be closed by this pull request

IBCModuleV2 Transfer Application Spike #7510

Open

4 tasks

gjermundgaraba added 2 commits November 15, 2024 14:39

Merge branch 'feat/ibc-eureka' into cian/issue#7510-ibcmodulev2-trans…

8a4fb46

…fer-application-spike

lint

468035e

gjermundgaraba requested review from AdityaSripal and chatton November 18, 2024 13:49

bznein changed the title ~~[WIP] Ibcmodulev2 transfer application spike~~ Ibcmodulev2 transfer application spike Nov 18, 2024

gjermundgaraba approved these changes Nov 18, 2024

View reviewed changes

gjermundgaraba mentioned this pull request Nov 18, 2024

Add full MsgServerV2 test with mock application #7372

Closed

AdityaSripal approved these changes Nov 18, 2024

View reviewed changes

AdityaSripal reviewed Nov 18, 2024

View reviewed changes

make signature consistent

1992fde

bznein merged commit 7ccd1c0 into feat/ibc-eureka Nov 19, 2024
64 of 65 checks passed

bznein deleted the cian/issue#7510-ibcmodulev2-transfer-application-spike branch November 19, 2024 09:16

gjermundgaraba mentioned this pull request Nov 22, 2024

E2E Transfer Eureka Test Happy Path #7210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ibcmodulev2 transfer application spike #7524

Ibcmodulev2 transfer application spike #7524

bznein commented Oct 29, 2024 •

edited by chatton

Loading

gjermundgaraba commented Oct 30, 2024

AdityaSripal left a comment

AdityaSripal Oct 31, 2024

chatton Nov 4, 2024

chatton commented Nov 4, 2024

gjermundgaraba commented Nov 18, 2024

gjermundgaraba left a comment

gjermundgaraba Nov 18, 2024

bznein Nov 18, 2024

AdityaSripal left a comment

AdityaSripal Nov 18, 2024

bznein Nov 18, 2024

AdityaSripal Nov 18, 2024

AdityaSripal Nov 18, 2024

sonarcloud bot commented Nov 19, 2024

Ibcmodulev2 transfer application spike #7524

Ibcmodulev2 transfer application spike #7524

Conversation

bznein commented Oct 29, 2024 • edited by chatton Loading

Description

Questions

gjermundgaraba commented Oct 30, 2024

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Nov 4, 2024

gjermundgaraba commented Nov 18, 2024

gjermundgaraba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 19, 2024

Quality Gate passed for 'ibc-go'

bznein commented Oct 29, 2024 •

edited by chatton

Loading