Skip to content
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

test: IBC transfer scenarios #10852

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

test: IBC transfer scenarios #10852

wants to merge 10 commits into from

Conversation

0xpatrickdev
Copy link
Member

closes: #XXXX
refs: #XXXX

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Copy link

cloudflare-workers-and-pages bot commented Jan 16, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 560a63a
Status: ✅  Deploy successful!
Preview URL: https://9195fab4.agoric-sdk.pages.dev
Branch Preview URL: https://pc-ibc-xfer-test.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev force-pushed the pc/ibc-xfer-test branch 2 times, most recently from 21dbe29 to 014071b Compare January 21, 2025 17:23
@michaelfig michaelfig changed the title test: ibc transfer scenarios test: IBC transfer scenarios Jan 23, 2025
@michaelfig michaelfig self-assigned this Jan 23, 2025
@michaelfig michaelfig added the force:integration Force integration tests to run on PR label Jan 23, 2025
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm confusing myself on which hook does what. Do we have a flow diagram somewhere?

Comment on lines +170 to +174
func (k Keeper) PacketStore(ctx sdk.Context, ourOrigin types.PacketOrigin, ourPort string, ourChannel string, sequence uint64) (storetypes.KVStore, []byte) {
key := fmt.Sprintf("%s/%s/%s/%v", ourOrigin, ourPort, ourChannel, sequence)
packetKey := []byte(key)
return prefix.NewStore(ctx.KVStore(k.key), []byte(packetDataStoreKeyPrefix)), packetKey
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make a reference to https://github.com/agoric-labs/ibc-go/blob/v6.3.1-alpha.agoric.2/modules/core/24-host/keys.go#L183-L187 but mention we need to support both directions (hence the extra origin)?

Btw I like the paths used by IBC better because it denotes what each path element is in the name

originalData/{src|dst}/ports/{ourPort}/channel/{ourChannel}/sequences/{sequence}

if StorePacketData && !bytes.Equal(strippedPacket.GetData(), packet.GetData()) {
packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
if !packetStore.Has(packetKey) {
packetStore.Set(packetKey, packet.GetData())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we remembering the packet data when receiving an ack? Shouldn't we assert we had saved packet data if we rewrote the packet, and delete instead?

origPacket := packet
packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
if packetStore.Has(packetKey) {
origPacket.Data = packetStore.Get(packetKey)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, why do we need to restore the original packet's data? Wouldn't a received timeout packet contain the original packet data in the first place?

packet.GetDestPort(), packet.GetDestChannel(),
timeout, packet.GetTimeoutTimestamp(),
)
packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're writing an ack, wouldn't we be the destination / receiver? Why are we using a src origin here?

@mhofman mhofman self-requested a review January 23, 2025 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants