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

[SC-1946] Move back to ethabi master #1237

Closed
morelazers opened this issue Jan 28, 2022 · 12 comments
Closed

[SC-1946] Move back to ethabi master #1237

morelazers opened this issue Jan 28, 2022 · 12 comments
Labels

Comments

@morelazers
Copy link

Description

We are currently using a custom fork of https://github.com/rust-ethereum/ethabi/ which has no-std compatibility.

rust-ethereum/ethabi#236 Seems to have sorted out the no-std issue we were having so we should test this out and move back to it.

@dandanlen
Copy link
Collaborator

dandanlen commented Jan 28, 2022

FWIW there are some other custom additions to our ethabi lib (which also might now be fixed in the main repo, tbc).

From memory - it was related to some dependency resolution issues / turning serialization on/off for ethereum primitives in dependent libs.

A quick win might be to merge the latest version of ethabi into our fork thereof...

@morelazers
Copy link
Author

I can give this a crack at some point, should I treat successful compilation as a victory?

@morelazers
Copy link
Author

A quick win might be to merge the latest version of ethabi into our fork thereof...

Easier said than done I think: chainflip-io/ethabi#2

@dandanlen
Copy link
Collaborator

Surprised to see so many conflicts... our changes were quite localised...

I can give this a crack at some point, should I treat successful compilation as a victory?

Yes I think so.

@morelazers
Copy link
Author

I might have used git wrong tbf, I did everything through github.com

@dandanlen
Copy link
Collaborator

Fyi. we forked off another fork (darwinia-network).

And our backend repo references a specific branch of our ethabi repo (https://github.com/chainflip-io/ethabi/tree/feature/with-ethereum-serialize).

@morelazers
Copy link
Author

Tried to open the correct PR here: chainflip-io/ethabi#3

@dandanlen
Copy link
Collaborator

Looking at it again - we already are up-to-date with darwinia-main on the main branch of our own ethabi repo. So we have already done some of the legwork.

I think the right thing to do is simply to merge the feature branch into main in our ethabi repo (there are conflicts tho). I can take a look on Monday.

@dandanlen
Copy link
Collaborator

Update: the change has been merged into ethabi master - should be part of the next release.

@morelazers
Copy link
Author

Looks like they have a v17.0.0 commit - it's not on their "releases" page yet though.

@dandanlen
Copy link
Collaborator

It's on crates.io, so we should be able to reference it from cargo.

I'll take another look.

@dandanlen
Copy link
Collaborator

No longer relevant as of #1650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants