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

internal/ethapi: merge CallArgs and SendTxArgs #22718

Merged
merged 2 commits into from
May 25, 2021

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Apr 22, 2021

There are two transaction parameter structures defined in the codebase, although for different purposes.
But most of the parameters are shared. So it's nice to reduce the code duplication by merging them together.

@fjl fjl changed the title eth, graphql, internal/ethapi: merge CallArgs and SendTxArgs internal/ethapi: merge CallArgs and SendTxArgs Apr 22, 2021
@fjl fjl added this to the 1.10.4 milestone May 11, 2021
@fjl fjl self-assigned this May 17, 2021
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

internal/ethapi: remove unused function
@holiman
Copy link
Contributor

holiman commented May 18, 2021

Previously, we had

// CallArgs represents the arguments for a call.
type CallArgs struct {
	From       *common.Address   `json:"from"`
	To         *common.Address   `json:"to"`
	Gas        *hexutil.Uint64   `json:"gas"`
	GasPrice   *hexutil.Big      `json:"gasPrice"`
	Value      *hexutil.Big      `json:"value"`
	Data       *hexutil.Bytes    `json:"data"`
	AccessList *types.AccessList `json:"accessList"`
}

And

type SendTxArgs struct {
	From     common.Address  `json:"from"`
	To       *common.Address `json:"to"`
	Gas      *hexutil.Uint64 `json:"gas"`
	GasPrice *hexutil.Big    `json:"gasPrice"`
	Value    *hexutil.Big    `json:"value"`
	Nonce    *hexutil.Uint64 `json:"nonce"`
	// We accept "data" and "input" for backwards-compatibility reasons. "input" is the
	// newer name and should be preferred by clients.
	Data  *hexutil.Bytes `json:"data"`
	Input *hexutil.Bytes `json:"input"`

	// For non-legacy transactions
	AccessList *types.AccessList `json:"accessList,omitempty"`
	ChainID    *hexutil.Big      `json:"chainId,omitempty"`
}

These are now replaced by

type TransactionArgs struct {
	From     *common.Address `json:"from"`
	To       *common.Address `json:"to"`
	Gas      *hexutil.Uint64 `json:"gas"`
	GasPrice *hexutil.Big    `json:"gasPrice"`
	Value    *hexutil.Big    `json:"value"`
	Nonce    *hexutil.Uint64 `json:"nonce"`

	// We accept "data" and "input" for backwards-compatibility reasons.
	// "input" is the newer name and should be preferred by clients.
	// Issue detail: https:/ethereum/go-ethereum/issues/15628
	Data  *hexutil.Bytes `json:"data"`
	Input *hexutil.Bytes `json:"input"`

	// For non-legacy transactions
	AccessList *types.AccessList `json:"accessList,omitempty"`
	ChainID    *hexutil.Big      `json:"chainId,omitempty"`
}

One thing to note here is that now, From *common.Address becomes an optional field, which can be nil. This may bite us somewhere, since we're now dealing with a pointer which we previously did not

@holiman
Copy link
Contributor

holiman commented May 18, 2021

Addendum: I think it's fine the way Gary handled it

@holiman
Copy link
Contributor

holiman commented May 20, 2021

@fjl want to TAL again? This one might bitrot if we don't merge it soonish

@fedekunze
Copy link

Hi, just wondering, is there any reason why this type is located on the internal package? I think it would be great to have all the RPC types public

@fjl
Copy link
Contributor

fjl commented May 25, 2021

@fedekunze The types in internal/ethapi are specific to the server side of the API.
We don't want to expose it to the public because the client side should not depend
on the server backend.

@fjl fjl merged commit 51b32cc into ethereum:master May 25, 2021
@fjl fjl removed the status:triage label May 25, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
There are two transaction parameter structures defined in
the codebase, although for different purposes. But most of
the parameters are shared. So it's nice to reduce the code
duplication by merging them together.

Co-authored-by: Martin Holst Swende <[email protected]>
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 5, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 26, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Aug 1, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 5, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 5, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 5, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Aug 5, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 6, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 7, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Aug 11, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Aug 12, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 12, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Aug 12, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 12, 2024
gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 19, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Aug 19, 2024
gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 21, 2024
wanwiset25 pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 23, 2024
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.

4 participants