-
Notifications
You must be signed in to change notification settings - Fork 581
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
Implement v2 RecvPacket rpc handler #7421
base: feat/ibc-eureka
Are you sure you want to change the base?
Implement v2 RecvPacket rpc handler #7421
Conversation
…nt-on-channelkeeperv2' into cian/issue#7354-implement-v2-recvpacket-rpc-handler
return errorsmod.Wrap(channeltypes.ErrInvalidPacket, "receipt not found for packet") | ||
} | ||
|
||
multiAckBz := k.cdc.MustMarshal(&ack) |
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.
rename to ackBz
newAttributes[j] = sdk.NewAttribute(coretypes.ErrorAttributeKeyPrefix+attribute.Key, attribute.Value) | ||
} | ||
|
||
newEvents[i] = sdk.NewEvent(coretypes.ErrorAttributeKeyPrefix+event.Type, newAttributes...) |
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 we can avoid having the newAttributes
slice if we do
newEvents := make(sdk.Events, len(events))
for i, event := range events {
newEvents[i] = sdk.NewEvent(coretypes.ErrorAttributeKeyPrefix+event.Type)
for _, attribute := range event.Attributes {
newEvents[i].AppendAttributes(sdk.NewAttribute(coretypes.ErrorAttributeKeyPrefix+attribute.Key, attribute.Value))
}
}
WDYT?
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 was just copy pasted from the existing fn in v1 msg_server.go, I'll create another issue to remove the duplication, and maybe we can make these changes here? I'll link this comment in the issue.
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.
ah sure no big deal!
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.
suite.Require().Contains(err.Error(), tc.expError.Error()) | ||
|
||
_, ok := ck.GetPacketReceipt(path.EndpointB.Chain.GetContext(), recvPacket.SourceChannel, recvPacket.Sequence) | ||
suite.Require().False(ok) | ||
ibctesting.RequireErrorIsOrContains(suite.T(), err, tc.expError) |
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 line 261 be at 257 (and the current 257 can be removed?)
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.
merge conflict mistake I think, will fix!
packet channeltypesv2.Packet, | ||
ack channeltypesv2.Acknowledgement, | ||
) error { | ||
// Lookup channel associated with our source channel to retrieve the destination channel |
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 source and destination be swapped here? Given that below we use DestinationChannel
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.
def can do with some retouching, I think the one for recv can be reused here just fine
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.
Not sure if we meant the same thing, I meant in the comment anyway :D
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.
ah think comment was from a copy pasta, will fix, nice catch
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
@@ -92,18 +101,66 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *channeltypesv2.MsgRecvPack | |||
return nil, errorsmod.Wrap(err, "invalid address for msg Signer") | |||
} | |||
|
|||
_ = signer | |||
switch err { | |||
case nil: |
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.
Just noticed one thing: this case can never be true, since we check for err != nil
above. I believe this is supposed to be a different err
(the one at line 92? But in that case we still don't need this case
)
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.
really nice catch! Also highlights the missing NoOp case, I'll push a fix for this
Quality Gate passed for 'ibc-go'Issues Measures |
Description
closes: #7354
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.