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

Transaction.gasPrice Optional #2396

Merged
merged 13 commits into from
Jun 22, 2021
Merged

Transaction.gasPrice Optional #2396

merged 13 commits into from
Jun 22, 2021

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Jun 9, 2021

Signed-off-by: garyschulte [email protected]

PR description

Systemic treatment of gasPrice as Optional has revealed several areas where we are still assuming gasPrice is not null and where we do not have specific treatment of 1559 fee fields

Fixed Issue(s)

#2389

Changelog

Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

LGTM

@matkt
Copy link
Contributor

matkt commented Jun 17, 2021

@garyschulte maybe you have to ask those who work on the privacy features to be sure that there is no impact

Signed-off-by: garyschulte <[email protected]>
Copy link
Member

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

Overall it is looking good and I don't think it will cause any side effects on private txs.

I've left a few comments/suggestions. Nothing major.

@@ -112,7 +112,8 @@ public MainnetTransactionValidator(
return signatureResult;
}

if (goQuorumCompatibilityMode && !transaction.getGasPrice().isZero()) {
if (goQuorumCompatibilityMode
&& !transaction.getGasPrice().map(Wei::isZero).orElse(Boolean.TRUE)) {
Copy link
Member

Choose a reason for hiding this comment

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

This clause is getting harder and harder to understand. Maybe we could encapsulate the "tx cost zero" logic into a method in the Transaction class?

Something like: transaction.hasGasPrice() or transaction.isGasPriceZero() (I'm not great with names, feel free to come up with a better name for this method).

I recon this would make this if statement easier to read:

if (goQuorumCompatibilityMode && transaction.hasGasPrice()) {
    ....
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

encapsulated in core Transaction. I went ahead and added 1559 fields just in case.

@lucassaldanha
Copy link
Member

One thing that I'm thinking that we might want is to simplify the access to the tx gas price when we don't want to handle the optional value (e.g. when optional.empty means zero and not-empty means a Wei value).

e.g.
return transaction.getGasPrice().orElse(Wei.ZERO) and transaction.getGasPrice().get().getValue().longValue()
could become:
return transaction.getGasPriceValue() and transaction.getGasPriceValue().longValue()

What I'm thinking is that having a method like transaction.getGasPriceValue() or something, that "hides" the optional value in favour of a Wei or numeric might simplify the access to this property in some places.

@garyschulte garyschulte enabled auto-merge (squash) June 21, 2021 23:33
@garyschulte garyschulte merged commit 2d0732a into hyperledger:master Jun 22, 2021
jflo pushed a commit to jflo/besu that referenced this pull request Jun 25, 2021
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
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