feat: add solidity ABI support in ICS20Transfer ics20-1 #7592
Conversation
| - name: Install compiler for arm64. | ||
| if: matrix.go-arch == 'arm64' | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-aarch64-linux-gnu | ||
| sudo apt-get install -y gcc-aarch64-linux-gnu g++-aarch64-linux-gnu build-essential |
There was a problem hiding this comment.
Some changes in dependencies has made this necessary
| @@ -40,6 +40,9 @@ issues: | |||
| - linters: | |||
| - staticcheck | |||
| text: "SA1019:" # silence errors on usage of deprecated funcs | |||
| - text: "G115: integer overflow conversion" | |||
There was a problem hiding this comment.
This is not strictly necessary currently, but we will need it as soon as we update to a newer version of golangci-lint. We use a newer one in the solidity repo (and 1.57 seems broken on my Mac, so I am running the same from the solidity repo), which turns on G115 by default. We need convert integers constantly, and there is no way to appease the linter (according to issues I found on gh). So if we just add it now, we'll save ourselves some pain when we upgrade golangci-lint.
| @@ -26,6 +27,7 @@ var ( | |||
| const ( | |||
| EncodingJSON = "application/json" | |||
| EncodingProtobuf = "application/x-protobuf" | |||
| EncodingABI = "application/x-solidity-abi" | |||
There was a problem hiding this comment.
I was not able to find an actual "standard" or widely accepted MIME type for solidity ABI encoded data. So I made this one... Let me know if you have a better suggestion.
There was a problem hiding this comment.
Yeah, I'm not sure what the expectation is here for mime types that don't really exist.
This would be my hesitation for using mime format rather than just defining our own enum, although, much of a muchness really.. only negative is if at some point a standard mime type is added for abi solidity
There was a problem hiding this comment.
Yeah, that is true :/ In that case we might just have to support both for a while. Well, this is feedback that @AdityaSripal can think about :)
There was a problem hiding this comment.
Yeah, I'm not sure what the expectation is here for mime types that don't really exist.
FWIW, applications/x-protobuf is not standard either, some use application/protobuf but there isn't an agreed upon standard. We set on this one with @AdityaSripal but I get your hesitation
gjermundgaraba
requested review from
AdityaSripal,
damiannolan,
srdtrk,
chatton,
DimitrisJim and
bznein
as code owners
There was a problem hiding this comment.
when was this file added? is this something needed for eureka? I don't think we have it on main
There was a problem hiding this comment.
It was added recently in #7559. It is currently needed for the Union 08-wasm Ethereum light client. I hope to remove this dependency as we move the light client 🙏
| @@ -26,6 +27,7 @@ var ( | |||
| const ( | |||
| EncodingJSON = "application/json" | |||
| EncodingProtobuf = "application/x-protobuf" | |||
| EncodingABI = "application/x-solidity-abi" | |||
There was a problem hiding this comment.
Yeah, I'm not sure what the expectation is here for mime types that don't really exist.
This would be my hesitation for using mime format rather than just defining our own enum, although, much of a muchness really.. only negative is if at some point a standard mime type is added for abi solidity
| @@ -19,12 +19,13 @@ require ( | |||
| github.com/cosmos/cosmos-sdk v0.50.10 | |||
| github.com/cosmos/gogoproto v1.7.0 | |||
| github.com/cosmos/ics23/go v0.11.0 | |||
| github.com/cosmos/solidity-ibc-eureka/abigen v0.0.0-20241124101149-3e2c74b736f4 | |||
There was a problem hiding this comment.
this is a separate go module, right?
maybe in future could be nice to have all encoding and protobuf stuff in the ibc repo, but that's probably quite far off right now
There was a problem hiding this comment.
Yes, it is generated code from the ABI definitions of the solidity EVM contracts (Solidity -> ABI -> go).
There was a problem hiding this comment.
I actually think that we should have the encoding/decoding logic here (or ibc repo I guess). I'd even rather use manually written code to abi encode/decode the packet data than to use autogenerated code from another repo. But for now, this is probably ok...
There was a problem hiding this comment.
There is a bigger discussion to be had on centralizing types and encoding. For now, this is at least relatively convenient and gives a direct link to where the correct definitions are.
|
| { | ||
| "failure: invalid type for protobuf", | ||
| func() { | ||
| packetDataBz = []byte("invalid") |
There was a problem hiding this comment.
[nit] maybe we can extract it somewhere since we use it in at least three places
Description
closes: #7591
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.