-
Notifications
You must be signed in to change notification settings - Fork 69
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
WIP: Solidity Implementation #12
Conversation
Wow, interesting work. I will cut a v0.2.0 with the current protobuf spec (which you used). Can you add some commands in the readme I can use to run the test code? |
bump @mossid @ethanfrey - this is very interesting (glad to review if you like). |
Thanks for the bump, I didn't notice this was ready |
@mossid Code looks good and good reuse of test cases. I need to dig in deeper, but a few meta-comments (on layout not solidity code) But first, can you remove WIP if this is ready for review? |
I don't like adding the ethereum dependency to go.mod/go.sum for the pure go implementation. It seems to be impossible to separate out release/test dependencies with go mod (golang/go#26913), but maybe you can do what I did for the other one, base your
I'm actually not sure about licenses since go-ethereum is GPL, so definitely do not want to import it in Apache license production code. I'm fine with it in the test code for solidity, but the import should be nowhere else. |
I can see the solidity code in Can you add a Makefile (or at least a README with the commands)? Nice to help for reproducing. Also if I change a line of solidity, I want to re-run the tests. |
Please add a job to |
I'll take a deeper code dive likely Monday. It would be great if you could do some of this cleanup first, so I just test the solidity implementation. I kind of feel that there is better support for testing solidity in javascript. I have used https://github.com/xf00f/web3x before as it was typescript and had some nice compilation steps and all, but I know there is a wide array of tools. Not to say the go-ethereum tests are not valid, just that it might be easier to do the code-test-debug cycle with another framework. In the end, the only real artifact of this is the *.sol files, not the test harness, so if it is documented and runs on the CI, I am fine with it. Just a suggestion if it is easier. |
Agreed, the ethereum imported code is used for test contract deployment and hash functions. It certainly can be separated and remove the dependency on root go.mod.
I used Native Dapps, will add a makefile.
👍
I am not that familiar with Javascript, I think we can use the current codebase and switch it to javascript later(I agree that javascript has better support on solidity testing and we should switch to it) |
@mossid The comment sounds good. Did you have time to work on this? (I know you've been busy with DevCon and IBC work) |
3344daf
to
56c49d1
Compare
Working on this PR for future ibc-eth integration |
@mossid I think the types changed since you worked on this. They are quite stable now. You may want to restart with proper types? |
Is it possible to resume this PR? |
@lightyear15 question: what EVM are you targeting? It would be interesting for the proof verification to happen in solidity (it just needs hashes, not sig verification). However, for this really to make sense in IBC world, the same chain would have to provide valid Merkle proofs that the other side could parse. ICS23 doesn't support the Patricia Merkle Trie of Ethereum 1.0 as a choice, as that was so under-specified and variable (the last hash may be a leaf node, may be an inner node with 2 leafs...). Al relevant code to handle that was GPL, and even copying that GPL code into a demo repo, I had a very hard time making a proof verifier... If you are looking at some Ethereum 2.0 implementation using SMT or such, that should be possible to represent with ics23 (I have not tried, but I looked at the spec when implementing ics23) |
Thanks @ethanfrey for the prompt response. Regarding ICS23 and Patricia Merkle Trie for Eth1.0, I already realised how infeasibile seems to be converting a Patricia Merkle proof into a ICS23-compliant However, I would argue that looking at the current implementation of cosmos/ibc-go module, this is not necessary. Nevertheless, from my observation, no code uses that method in the ClientState interface. As a matter of fact, specification document for ICS06-solomachine does not mention anything about ICS23-compliant commitment proof and code shows that the function is simply ignored. In my hypothetical use case, where I am adding new client type into the cosmos/ibc-go module, the client can ignore the Am I completely off the track? |
Solo machines are based on trusting one signature. I mean, kind of like a poor bridge (unless you use threshold crypto to shard that signature). You should not use that design for blockchain-blockchain. We could add some custom Ethereum Patricia Trie verification functions in all 3 languages (after spec'ing it out well) and then make a new release with a version bump. This version bump would need to be tied to a breaking change in the ibc implementation (eg ibc-go 3). Then one could use that. |
I would check with the IBC team from IG maybe they have some ideas I haven't thought about |
Thanks @ethanfrey, It was easier that way rather than asking you to git clone the repo and grep for Thanks again, |
Hi @ethanfrey , |
I have not reviewed this code, but if you want to take this over (or start from scratch using this as a reference), I am happy for the contribution. If there are some CI tests for Solidity, that would be great |
Hello @lightyear15 , thank you for the detailed writeup. As you say, we do not currently use We do use the We discussed keeping the interface public so that we can use it when we relax the We are planning to greatly reduce the interface for For now, you can ignore the |
Closing in favour of #58 |
No description provided.