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

feat: support uint256 for rlp, log, hex, vm, abi #233

Merged
merged 8 commits into from
Jan 31, 2025

Conversation

tnasu
Copy link
Contributor

@tnasu tnasu commented Jan 24, 2025

Proposed changes

Support uint256 for RLP, Hex, etc. These changes also help to update EIP-7702, which uses uint256

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have read the CLA and signed by comment I have read the CLA Document and I hereby sign the CLA in first time contribute
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

@tnasu tnasu self-assigned this Jan 24, 2025
@tnasu tnasu marked this pull request as ready for review January 24, 2025 10:38
blukat29
blukat29 previously approved these changes Jan 28, 2025
rlp/rlpgen/gen.go Outdated Show resolved Hide resolved
@ulbqb
Copy link
Contributor

ulbqb commented Jan 30, 2025

Why is this needed? Doesn't the other txs use uint256?

@tnasu
Copy link
Contributor Author

tnasu commented Jan 30, 2025

@ulbqb

Doesn't the other txs use uint256?

What are the other txs? If the other txs isn't SET_CODE_TX(EIP-7702), then there is no plan to use uinit256 for the other txs. If we want to use uint256, we should support uint256 like this PR.
On the other hand, I can also say this PR isn't needed since we can use big.Int instead of uint256.

But, I think using uint256 is popular usage in Geth and the others since it is better performance than big.Int. Why don't we use it?

What is your opinion?

Thanks

@ulbqb
Copy link
Contributor

ulbqb commented Jan 30, 2025

On the other hand, I can also say this PR isn't needed since we can use big.Int instead of uint256.

I see. I was curious about if type 1 or 2 have already used uint256 fields. IMO, uint256 should be applied for the other txs unless there is huge impact. If it will be done by geth, there is no reason why we don't apply.

@tnasu
Copy link
Contributor Author

tnasu commented Jan 31, 2025

@ulbqb

I was curious about if type 1 or 2 have already used uint256 fields

We don't use uint256 for existing transaction types because of need this PR.

uint256 should be applied for the other txs unless there is huge impact

It's hard to say whether it has a huge impact or not. I'm making this PR by searching Geth PRs. It's hard to introduce using uint256, and we have to maintain related uint256 after merging this PR.
https://github.com/search?q=repo%3Aethereum%2Fgo-ethereum+uint256++is%3Amerged&type=pullrequests&p=1

If it will be done by geth, there is no reason why we don't apply.

Geth is using uint256 for txs after blob_tx. The other txs are using bigInt

@tnasu tnasu merged commit 27124ed into kaiachain:dev Jan 31, 2025
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2025
@tnasu tnasu deleted the dev-uint256 branch February 4, 2025 06:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants