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

Add chain_id when building transaction #58

Closed
wants to merge 2 commits into from
Closed

Add chain_id when building transaction #58

wants to merge 2 commits into from

Conversation

dimitarvp
Copy link
Contributor

@dimitarvp dimitarvp commented Oct 25, 2021

Task list

  • Add chain_id to Builder
  • Fix bug when decoding the chain_id -- allow it to be larger than one byte
  • Add chain_id to Signer

@dimitarvp
Copy link
Contributor Author

I started getting this error when working with Polygon Mumbai (Polygon testnet):

%{"code" => -32000, "message" => "only replay-protected (EIP-155) transactions allowed over RPC"}

I tried adding the chain_id to the builder but it seems it's not enough. @izelnakri I'll try modify the transaction signer as well. This PR is not ready for merge.

@dimitarvp
Copy link
Contributor Author

dimitarvp commented Oct 25, 2021

This commit -- 5c3860f -- allows the encoded chain_id be more than 1 byte.

The previous code fails when chain_id is 80001 (Polygon Mumbai (testnet)):

<<number>> = to_buffer(data)

(data in this case is <<2, 113, 37>>, which is 2*80001 + 35 encoded as bytes; this is required by EIP-155)

But by using this:

data |> to_buffer() |> :binary.decode_unsigned()

We allow the result to be an arbitrarily long integer (as EIP-155 specifies).

@dimitarvp
Copy link
Contributor Author

@dimitarvp dimitarvp marked this pull request as draft October 25, 2021 14:10
@izelnakri
Copy link
Owner

The changes so far looks good to me, I’ll read your comments in depth ;)

Meanwhile Im thinking we might want to improve/refactor CI to use openethereum first, what do you think?

@dimitarvp
Copy link
Contributor Author

I know nothing about it so far. Could read on it after this is completed.

@dimitarvp
Copy link
Contributor Author

This is not ready for merge, no need to approve yet. I'll appreciate help on fixing the signer code -- although I am working on it myself and should be done soon.

@quangacuity
Copy link

quangacuity commented Apr 1, 2022

This is not ready for merge, no need to approve yet. I'll appreciate help on fixing the signer code -- although I am working on it myself and should be done soon.

Hi @dimitarvp, any updates on this?

I'm trying to continue with your work here quangacuity@48639ba. But this doesn't seem to be enough as I get this error now when trying to send the signed transaction.

{:error, %{"code" => -32000, "message" => "invalid sender"}}

@izelnakri Do you have any suggestions on how to make this work for ETH.Transaction.Signer?

Many thanks.

Edited: Ok I manage to get it working by this commit.

@izelnakri
Copy link
Owner

Hi @quangacuity , we need to change the CI entirely, it currently uses ganache-cli but I'd much rather like to use the test environment of openethereum first. Because we had mismatches in the past. at least relying on a real implementation/production code is what we should strive for going forward.

@dimitarvp dimitarvp closed this by deleting the head repository Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants