Skip to content

Conversation

@karalabe
Copy link
Member

No description provided.

@karalabe karalabe added this to the 1.10.8 milestone Aug 13, 2021
@karalabe karalabe requested review from fjl and holiman August 13, 2021 09:55
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, the canonical zero check we use everywhere is int.Sign() == 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

Copy link
Contributor

@fjl fjl Aug 13, 2021

Choose a reason for hiding this comment

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

Here, tx.ChainId can never be nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

// However, if the user asked for a particular chain id, then we should
// use that instead.
if tx.ChainId() != nil {
if tx.ChainId().Sign() != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need a nil-check here?

Copy link
Contributor

Choose a reason for hiding this comment

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

ChainId() cannot return nil for non-legacy transactions.

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

@karalabe karalabe merged commit 0a68558 into ethereum:master Aug 13, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…reum#23394)

* accounts/external: handle 0 chainid as not-set for the Clef API

* accounts/external: document SignTx

Co-authored-by: Felix Lange <[email protected]>
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