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 refund to execution result in Go binding #690

Merged

Conversation

HerbertJordan
Copy link
Contributor

@HerbertJordan HerbertJordan commented May 5, 2023

This PR modifies the Execute() function from the Go bindings to return a Result struct instead of a list of result values to support the retrieval of the amount of refunded gas. This information is available in evmc.h's evmc_result, but was not exposed in the Go binding so far.

Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @HerbertJordan, thanks for the contribution.

I'd rather change the existing API of Execute() to return Result as you proposed. What do you think?

@HerbertJordan
Copy link
Contributor Author

HerbertJordan commented May 8, 2023

Hi @chfast, thanks for reviewing this PR.

If backward compatibility is not an issue, we should indeed update the existing Execute() method. In that case, may I also suggest replacing the parameter list with a parameter struct for the same reasons as for the result (see updated PR)?

@HerbertJordan HerbertJordan force-pushed the herbert/include_refund_in_go_binding branch from 8e42f42 to 65da3fc Compare May 8, 2023 06:37
@HerbertJordan HerbertJordan requested a review from chfast May 8, 2023 06:52
@HerbertJordan HerbertJordan force-pushed the herbert/include_refund_in_go_binding branch from 65da3fc to 153f1b0 Compare May 11, 2023 05:54
@HerbertJordan HerbertJordan force-pushed the herbert/include_refund_in_go_binding branch from 153f1b0 to 8b80acb Compare July 31, 2023 15:18
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #690 (c221cf2) into master (c3feb59) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #690   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files          25       25           
  Lines        3857     3857           
  Branches      396      396           
=======================================
  Hits         3606     3606           
  Misses        139      139           
  Partials      112      112           

bindings/go/evmc/evmc.go Outdated Show resolved Hide resolved
@HerbertJordan HerbertJordan force-pushed the herbert/include_refund_in_go_binding branch from 3073588 to 7641dfe Compare August 10, 2023 12:03
@chfast chfast force-pushed the herbert/include_refund_in_go_binding branch from 7641dfe to c221cf2 Compare November 29, 2023 09:05
@chfast chfast merged commit bf7f382 into ethereum:master Nov 29, 2023
20 checks passed
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.

2 participants