Ibcmodulev2 transfer application spike

bznein commented 3 weeks ago

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.

[ ] Targeted PR against the correct branch (see CONTRIBUTING.md).
[ ] Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
[ ] Code follows the module structure standards and Go style guide.
[ ] Wrote unit and integration tests.
[ ] Updated relevant documentation (docs/).
[ ] Added relevant godoc comments.
[ ] Provide a conventional commit message to follow the repository standards.
[ ] Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
[ ] Re-reviewed Files changed in the GitHub PR explorer.
[ ] Review SonarCloud Report in the comment section below once CI passes.

gjermundgaraba commented 3 weeks ago

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?

chatton commented 3 weeks ago

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.

gjermundgaraba commented 1 week ago

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