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

conversion: revert json-marshalling change in #144, marshal hex-format #145

Closed
wants to merge 1 commit into from

Conversation

holiman
Copy link
Owner

@holiman holiman commented Nov 30, 2023

This undoes the change in json-marshalling, making it so that we (again) marshal uint256.Int in hex-format. This is more widely useful in go-ethereum.

I originally intended to let it be this way, but I changed my mind due to this comment: #144 (comment).

This would be an alternative to ethereum/go-ethereum#28628

cc @fjl @karalabe

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #145 (add7529) into master (f24ed59) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #145   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1643      1643           
=========================================
  Hits          1643      1643           

@karalabe
Copy link
Contributor

karalabe commented Nov 30, 2023 via email

@holiman
Copy link
Owner Author

holiman commented Nov 30, 2023

Because in many places we jump through hoops in order to force our big.Int's to output hex. E.g. ethereum/go-ethereum#28628.

@holiman
Copy link
Owner Author

holiman commented Nov 30, 2023

As @fjl noted:

However, let me just say I find the upstream change is not the best. We always want hex output in go-ethereum. So now we need to add a conversion every time just like for big.Int. I thought uint256 is purpose built for our use, why would we make it harder for ourselves?

@holiman holiman closed this Dec 4, 2023
@lmittmann
Copy link

What is the status here? will this change from hex to decimal format be reverted or not? Having decimal marshaling is kind of annoying in the ethereum context and would require a proxy type as hexutil.Big everywhere.

@holiman
Copy link
Owner Author

holiman commented Dec 9, 2023 via email

@lmittmann
Copy link

Okay. Just out of curiosity, what is the reason for changing this in the first place?

To give some more context: My lib is effected by this breaking change (I presume it will be relatively easy to fix though). But still, this package has hundreds of direct dependents which code might break or act unexpected. This should be a v2 release IMO or kept as is.

@holiman
Copy link
Owner Author

holiman commented Dec 12, 2023

Okay. Just out of curiosity, what is the reason for changing this in the first place?

To be more like big.Int. Or exactly which of the changes are you referring to?

To give some more context: My lib is effected by this breaking change (I presume it will be relatively easy to fix though). But still, this package has hundreds of direct dependents which code might break or act unexpected. This should be a v2 release IMO or kept as is.

Well, depends on how you see it. It doesn't break the programing API, but yes, it does break some places where marshalling is concerned. But many places where you would unmarshal a hex-string, would also accept an int-string.

Anyway, go-ethereum is the main driver for this library.

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