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

Ack & timeout relay msgs #1810

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Ack & timeout relay msgs #1810

merged 2 commits into from
Sep 26, 2024

Conversation

grod220
Copy link
Collaborator

@grod220 grod220 commented Sep 25, 2024

A follow up from: #1805

Able to display IBC relay acknowledgements & timeouts in the transaction details page:

Screenshot 2024-09-25 at 10 37 04 AM Screenshot 2024-09-25 at 10 38 29 AM

Copy link

changeset-bot bot commented Sep 25, 2024

⚠️ No Changeset found

Latest commit: c40814d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines +43 to +50
const PacketRows = ({
packet,
dataParser,
}: {
packet: Packet;
dataParser?: (arg: Uint8Array) => ReactElement;
}) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because a number of messages need to display Packet data, abstracted it here

@grod220 grod220 requested a review from a team September 25, 2024 08:48
@grod220 grod220 self-assigned this Sep 25, 2024
@@ -140,5 +232,23 @@ export const IbcRelayComponent = ({ value }: { value: IbcRelay }) => {
return <UpdateClientComponent update={update} />;
}

if (value.rawAction?.is(MsgTimeout.typeName)) {
const timeout = new MsgTimeout();
value.rawAction.unpackTo(timeout);
Copy link
Contributor

@TalDerei TalDerei Sep 26, 2024

Choose a reason for hiding this comment

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

suggestion: since the proto definition of rawAction is Any, could there potentially be errors when unpacking? It might be a good idea to wrap the unpackTo method in a try-catch block when deserializing the rawAction into a message type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given the conditional block it's in, it should unbpack fine. However, we want this to be fault-tolerant for unexpected values or deserialization errors. Will wrap in try/catch.

return (
<>
<ParsedPacketData data={packet.data} dataParser={dataParser} />
{!!packet.sequence && (
Copy link
Contributor

@TalDerei TalDerei Sep 26, 2024

Choose a reason for hiding this comment

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

comment: what does a packet sequence number of 0 represent? and if it's valid, wouldn't that be treated as falsy? are there other such instances like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, will change any field that returns a primitive (required field) to show without a conditional check.

return (
<>
<ParsedPacketData data={packet.data} dataParser={dataParser} />
{!!packet.sequence && (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, will change any field that returns a primitive (required field) to show without a conditional check.

@@ -140,5 +232,23 @@ export const IbcRelayComponent = ({ value }: { value: IbcRelay }) => {
return <UpdateClientComponent update={update} />;
}

if (value.rawAction?.is(MsgTimeout.typeName)) {
const timeout = new MsgTimeout();
value.rawAction.unpackTo(timeout);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given the conditional block it's in, it should unbpack fine. However, we want this to be fault-tolerant for unexpected values or deserialization errors. Will wrap in try/catch.

};

// Packet data stored as json string encoded into bytes
const parseRecvPacket = (packetData: Uint8Array) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was informed that all of these messages expect to have a FungibleTokenPacketData inside. Will refactor a bit for that case.

@grod220 grod220 merged commit d0c83ce into main Sep 26, 2024
6 checks passed
@grod220 grod220 deleted the additional-relay-msgs branch September 26, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants