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

Remove destinationAddress and destinationChainID from Warp Message #920

Merged

Conversation

nytzuga
Copy link
Contributor

@nytzuga nytzuga commented Sep 28, 2023

Why this should be merged

Remove destinationChainID and destinationAddress from the payload. Other libraries can implement their own mechanism to make the messages specific instead of anycast. More context in #868

How this works

Remove any reference to destinationChainID and destinationAddress

How this was tested

Unit tests and pipeline

How is this documented

Internal documentation will be updated to reflect changes #868

@nytzuga nytzuga force-pushed the warp-remove-destinationChainID-destinationAddress branch from 599baa3 to f76be8c Compare September 28, 2023 21:17
@nytzuga nytzuga marked this pull request as draft September 28, 2023 21:18
@nytzuga nytzuga force-pushed the warp-remove-destinationChainID-destinationAddress branch from f76be8c to b4a99ff Compare September 28, 2023 21:18
@nytzuga nytzuga marked this pull request as ready for review September 28, 2023 22:08
Copy link
Collaborator

@aaronbuchwald aaronbuchwald left a comment

Choose a reason for hiding this comment

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

Could you update the total size of the addressed payload in the README now that these two fields are removed? After that, this change LGTM

address indexed sender,
bytes message
);
event SendWarpMessage(address indexed sender, bytes message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: since we removed two index fields with this change, we could include the unsigned warp message ID if we want to.

This should be a separate change if we want it though. cc @michaelkaplan13 @ceyonur

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup that's what I thought after seeing this lol. It would be a great change!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that'd be a great idea. To confirm, that's the ID used for querying validators for their signature of a specific message right?

warp/payload/README.md Show resolved Hide resolved
Copy link
Collaborator

@darioush darioush 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 the code change looks good.
The change itself makes it such that applications must emit logs for destination chain and address if indexing is desired. I think we could make it clear in the readme that the typical application will need to encode the destination chain and contract on sending then verify this information as the first step in processing.

tests/warp/warp_test.go Outdated Show resolved Hide resolved
@nytzuga nytzuga self-assigned this Sep 29, 2023
@ceyonur ceyonur added this to the v0.5.7 milestone Oct 2, 2023
@nytzuga nytzuga force-pushed the warp-remove-destinationChainID-destinationAddress branch 3 times, most recently from 82e7cff to fd00831 Compare October 2, 2023 19:29
@nytzuga nytzuga force-pushed the warp-remove-destinationChainID-destinationAddress branch from fd00831 to 8d8de76 Compare October 2, 2023 19:33
Copy link
Collaborator

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

Could we regenerate the precompile to see if we need to modify packers/unpackers for changed inputs?

x/warp/contract.go Outdated Show resolved Hide resolved
The generated code now matches the output of generate_procompile.sh script
@aaronbuchwald aaronbuchwald merged commit f8bf348 into master Oct 3, 2023
@aaronbuchwald aaronbuchwald deleted the warp-remove-destinationChainID-destinationAddress branch October 3, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants