Skip to content

Conversation

@klaidliadon
Copy link
Contributor

@klaidliadon klaidliadon commented May 23, 2025

  • Update deprecated terminal package
  • Simplify PaddedAddress function by using TrimPrefix
  • Update blockLogsCount function to use builtin max
  • Simplify byte slice creation in solidityArgumentPack function
  • Implement JSON marshaling for various types using QuoteString utility
  • Avoid byte->string->byte conversion in hex encoding in MarshalText and SaveECDSA functions
  • Change TransactionsHash function to accept a pointer to types.Block since it has atomic values

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces minor improvements across utilities and Ethereum types, focusing on code simplification, performance optimizations, and adding JSON marshaling support.

  • Added a util.QuoteString helper and applied it to implement MarshalJSON for many types.
  • Replaced terminal with x/term for password reading and optimized hex encoding to avoid intermediate string allocations.
  • Simplified helper functions (PaddedAddress, solidityArgumentPack, blockLogsCount) and updated TransactionsHash signature.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
util/quote.go New QuoteString for JSON quoting
go.mod Added direct golang.org/x/term requirement
go-ethereum/rpc/types.go MarshalJSON for BlockNumber
go-ethereum/crypto/kzg4844/kzg4844.go MarshalJSON for Blob, Commitment, Proof
go-ethereum/crypto/crypto.go Optimized SaveECDSA hex encoding
go-ethereum/core/types/bloom9.go MarshalJSON for Bloom
go-ethereum/core/types/block.go MarshalJSON for BlockNonce
go-ethereum/core/types/account.go MarshalJSON for storageJSON
go-ethereum/common/types.go MarshalJSON for Hash, Address, UnprefixedAddress
go-ethereum/common/math/integer.go MarshalJSON for HexOrDecimal64
go-ethereum/common/math/big.go MarshalJSON for HexOrDecimal256, Decimal256
go-ethereum/common/hexutil/json.go MarshalJSON for Bytes, Big, U256, Uint64, Uint
ethreceipts/ethreceipts.go Simplified blockLogsCount with max
ethcoder/solidity_pack.go make([]byte, size) instead of make(..., size, size)
ethcoder/ethcoder.go PaddedAddress uses TrimPrefix
cmd/ethkit/wallet.go Swapped terminal.ReadPassword for term.ReadPassword
cmd/ethkit/block.go TransactionsHash now accepts *types.Block
Comments suppressed due to low confidence (3)

util/quote.go:6

  • There are no unit tests for QuoteString; consider adding tests to validate both successful quoting and error propagation from the underlying MarshalText.
func QuoteString(b encoding.TextMarshaler) ([]byte, error) {

ethreceipts/ethreceipts.go:945

  • [nitpick] The comment // this is unused is ambiguous; if blockLogsCount is truly unused, remove the function, otherwise delete or clarify the comment to improve readability.
// this is unused

ethreceipts/ethreceipts.go:949

  • This uses the built-in max which requires Go 1.21+; ensure the project’s minimum Go version supports these generic built-in functions, or explicitly import math if compatibility is needed.
blockCount = max(blockCount, log.TxIndex+1)

Comment on lines +11 to +13
raw = append([]byte{'"'}, raw...)
raw = append(raw, '"')
return raw, nil
Copy link

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

QuoteString wraps raw text in quotes but does not escape internal characters; for robust JSON encoding, consider using strconv.Quote or encoding/json’s Marshal to properly handle escape sequences.

Suggested change
raw = append([]byte{'"'}, raw...)
raw = append(raw, '"')
return raw, nil
quoted := strconv.Quote(string(raw))
return []byte(quoted), nil

Copilot uses AI. Check for mistakes.
@pkieltyka pkieltyka marked this pull request as draft May 30, 2025 19:19
@pkieltyka
Copy link
Member

thanks for the PR, but most changes are in go-ethereum which is a vendored library, so going to keep. for the terminal pkg, thanks for letting me know, I'll update it in another PR im working on

@pkieltyka pkieltyka closed this Jun 4, 2025
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