Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add initial integration (POC) of the axelar-encoding crate #22
feat: add initial integration (POC) of the axelar-encoding crate #22
Changes from 1 commit
0c3c7cf
12c37e1
3812bbe
366d837
f7f42ee
1766b3b
23be585
a30bd56
c38f997
6ec5af1
236a43e
394afe7
12d3991
cb1fce0
7c0e2fb
3659af1
0814a1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 would need to deeper check if this strings representations are really the expected ones in the other end.
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.
So all this fields, and it's related types, can be built again from a
String
representation. So obviously we are doing good here, as we are being agnostic. The decoding part would need know beforehand how to represent the decodedString
on respective types. Where is that decoding part ? I am missing something.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.
Maybe this is a fait point in that the
axelar-rkyv-encoding
crate should operate on the originalrouter_api::Message
types rather than Strings?I am looking at how the flow goes:
destination_address
.When I was working on the "e2e" tests between evm and solana, I was relying on the Pubkey::to_string() implementation when being invoked on the source chain (here's the decoding). iirc Pukey::to_string() is a base58 encoded string.
Where is the issue: We force the origin chain to produce strings of "destination_address" that must be properly parsed by the destination chain / destination relayer.
Solution: nothing we can really do here? Because theoretically its up to the caller on the source chain to properly encode
destination_address
so that we can parse it. Anyone can call a source gateway and pass destination addresses as hex encoded, binary encoded strings, base58 encoded strings, etc. And we cannot realistically handle this.And if we want this encoding be generic, then we cannot force it to use base58 encoded strings for ed25519 addresses either. This means that the parsing of the string gets propagated to the relayer.
Unless we step back from this encoding being Solana-agnostic: this encoding is now Solana-specific and all incoming
destination_address
params must be(base58|hex|something else)
encoded strings. Then we can add this enforcement in the encoding library and make this restriction explicit.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.
What you are doing here is just transforming the outer types, but the actual work will be performed by
rkyv
, so we don't need to worry about any manual serialization or deserialization. It's just likeserde
, where the type system will do all the heavy lifting for us.The original Axelar/CosmWasm types are all
String
s in the end, so I'm not worried about data loss — as long as we don't forget any data point when transforming the types into the ones inaxelar-rkyv-encoding
.True.
Well, since we are in control of the final types, we can force the
destination_address
to be[u8; 32]
, just likesolana_program::pubkey::Pubkey
. The fit will be even tighter.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.
Are the different parts of a same flow (chain scoped, so integration scoped) able to know (by agreement) how to properly handle the data at each stage right ? If yes, i would probably leave the encoding crate chain agnostic.
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.
So far, the
axelar-rkyv-encoding
is independent of Solana, and I'll try my best to ensure it stays that way. Importing any Solana dependencies would mean bringing in the whole circus of Solana crates as Amplifier dependencies, and I want to avoid that.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.
The benefits of this:
On the other hand the interface for the EVM Gateway expects that the address is already provided as a string rather than raw bytes.
iirc I tried doing exactly this (trying to send raw bytes, then converting them to a String inside Solidity, then sending it to the EVM gateway). But I had major issues with paring the events - the data didn't make sense. I think this is because ethers library generates the interfaces and types for events, and it expects the string parameter to be a valid UTF-8 string - but Solidity does not enforce strings to be utf8 hence the issue in parsing.
I think that we should rather leave the parameter as a String and keep the parsing logic on the relayer - with the assumption that the origin chain always sends base58 encoded to parse string as base58 for Solana. I think its easy to reason with this assumptoin. Even now, every gmp packet is expected to uphold some arbitrary encoding enforced by the destination relayer.
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 need to get rid of unwraps on this code
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 fixed at 12c37e1 , although i still think the error handling should ideally provide more details. Propagating errors up in the call stack without references can be confusing when their are shown on an log line.