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

Test/test execution http rpc #88

Closed
wants to merge 15 commits into from

Conversation

18aaddy
Copy link

@18aaddy 18aaddy commented Oct 29, 2024

Solves #87

  • Tested http_rpc_test.go locally and made modifications according to it in some files.
  • Tests working locally (other than CreateAccessList() function, which is not working, I'll raise an issue for that)

Summary of changes:

  • Most changes have been made in functions in http_rpc.go as the rpc data that is being sent and received is in form of hex strings. So changes had to be made in the way data was sent and also some structs for receiving data
  • I have updates the Transaction struct in common/types.go as otherwise, the data was not being unmarshaled. Due to this, I had to update few lines in consensus/consensus.go. I have tested the consensus part after this change and it works.
  • Also I had to change the execution/rpc.go file so that the ExecutionRpc interface is satisfied by HttpRpc

More explanation can be found in comments above the changes
Also the lines of code are increased because of go.mod and go.sum

@gerceboss
Copy link
Contributor

@18aaddy , can you pass the checks first?

Error: execution/execution.go:34:11: invalid operation: cannot indirect r (variable of type ExecutionRpc)
Error: execution/execution.go:126:16: cannot use proof.Nonce (variable of type hexutil.Uint64) as uint64 value in struct literal

Do you think a local command is needed for build and testing? So that you can comfortably check locally if those things do pass?

@18aaddy
Copy link
Author

18aaddy commented Oct 29, 2024

@gerceboss I'm not sure why the checks are failing as I don't even have execution/execution.go in the files in this PR

@gerceboss
Copy link
Contributor

Try rebasing

@18aaddy
Copy link
Author

18aaddy commented Oct 29, 2024

To resolve errors, I had to update the execution.go file, which is giving a merge conflict. The error in the execution.go file will be solved in PR #96 where I have modified the execution.go file

@gerceboss
Copy link
Contributor

execution.go by @ABD-AZE is the one that we will follow, please first understand that code and then add the modifications if any

@18aaddy
Copy link
Author

18aaddy commented Oct 30, 2024

I understand the code, there were some errors I found while testing, and thus I made modifications
In my final execution_test PR, all my final changes are there. That one will work once the CreateAccessList() function is modified. Then all tests will pass

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