-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ibcmodulev2 transfer application spike #7524
Ibcmodulev2 transfer application spike #7524
Conversation
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 :)
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked mainly at the high level interfaces. We want the sequence in ack and timeout as well.
@@ -27,6 +27,7 @@ type IBCModule interface { | |||
ctx context.Context, | |||
sourceChannel string, | |||
destinationChannel string, | |||
sequence uint64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added to OnAcknowledgementPacket
and OnTimeoutPacket
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep makes sense!
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
…fer-application-spike
…fer-application-spike
…fer-application-spike
For some context: this PR is being split up in such a way that forwarding support will happen in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good! And I've tested it e2e with solidity already, so happy path should be working at the very least :)
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an issue created for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, i have a couple suggestions but not blocking ack. I'm also a bit confused with how the app encoding logic
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be "proto"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
testing/mock/v2/ibc_app.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fn signature looks different than the fn outside the testing package
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", signer) | ||
} | ||
|
||
data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh because you aren't using the payload.Encoding
here
Quality Gate passed for 'ibc-go'Issues Measures |
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.
transfer/v2
. This keeper embeds the existing transfer/v1 keeper, and overrides functions required. E.g. the new OnRecv, Ack, Timeout etc.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.This test ensures that fungibility remains and tokens can be sent back and forth using either the v1 or v2 router.
Questions
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.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.