Skip to content

add MustFromBig for nicer struct initialization#128

Merged
holiman merged 4 commits intoholiman:masterfrom
karalabe:must
Mar 21, 2023
Merged

add MustFromBig for nicer struct initialization#128
holiman merged 4 commits intoholiman:masterfrom
karalabe:must

Conversation

@karalabe
Copy link
Copy Markdown
Contributor

@karalabe karalabe commented Mar 20, 2023 •
edited
Loading

This PR allows the second form (last two lines) along side the current form (first two lines) of initializing new ints from bigs.

var (
	costCap, _   = uint256.FromBig(item.Tx.Cost())
	gasTipCap, _ = uint256.FromBig(item.Tx.GasTipCap())
)
meta := &blobTxMeta{
	id:            id,
	size:          size,
	nonce:         item.Tx.Nonce(),
	costCap:       costCap,
	gasTipCap:     gasTipCap,
	gasFeeCap:     uint256.MustFromBig(item.Tx.GasFeeCap()),
	blobGasFeeCap: uint256.MustFromBig(item.Tx.BlobGasFeeCap()),
}

Panics can only be captured when a goroutine is unwinding, hence why the messy construct in the tests. Also, it would have been a tad cleaner to wait on the panic with sync.WaitGroup vs a channel, but the channel approach introducing a dependency on sync.

holiman
holiman reviewed Mar 21, 2023
View reviewed changes
holiman
holiman reviewed Mar 21, 2023
View reviewed changes
@karalabe
Copy link
Copy Markdown
Contributor Author

karalabe commented Mar 21, 2023

Actually, might have been nicer to just call FromBig and handle the return, more future proof.

@holiman
Copy link
Copy Markdown
Owner

holiman commented Mar 21, 2023

Actually, might have been nicer to just call FromBig and handle the return, more future proof.

You mean in this PR or from your side?

@holiman
Copy link
Copy Markdown
Owner

holiman commented Mar 21, 2023

Actually, I'm thinking that if you're in this case:

gasFeeCap:     uint256.MustFromBig(item.Tx.GasFeeCap()),

And let's say we know nothing about item.Tx at this point. Then it would be better to use a method which silently just overflows, not overflows+panic. Because if this happens early in the stack before the tx is properly validated, it is better to obtain a "wrong sender" than crashing the app.

So wouldn't you rather have a uint256.TryFromBig or something like that?

holiman
holiman reviewed Mar 21, 2023
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2023 •
edited
Loading

Codecov Report

Merging #128 (416c76a) into master (c385c61) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #128   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1538      1543    +5     
=========================================
+ Hits          1538      1543    +5

@karalabe
Copy link
Copy Markdown
Contributor Author

karalabe commented Mar 21, 2023

So wouldn't you rather have a uint256.TryFromBig or something like that?

Not really, all these data are already parsed from disk or network into uint256, only the tx interfaces are still using big.Int and I don't want to overhaul all the interfaces for the blob pool.

holiman
holiman approved these changes Mar 21, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@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

@holiman holiman changed the title Add support for MustFromBig for nicer struct initialization add MustFromBig for nicer struct initialization Mar 21, 2023
@holiman holiman merged commit 87b9142 into holiman:master Mar 21, 2023
@holiman holiman mentioned this pull request Mar 22, 2023
Merged
holiman added a commit that referenced this pull request Mar 22, 2023
@holiman
implement MustFromHex, MustFromDecimal (#131)
29b48c8 
The PR #128 added MustFromBig, this PR adds two similar helpers: MustFromHex and MustFromDecimal.

	// MustFromHex is a convenience-constructor to create an Int from
	// a hexadecimal string.
	// Returns a new Int and panics if any error occurred.
	func MustFromHex(hex string) *Int
	
	// MustFromDecimal is a convenience-constructor to create an Int from a
	// decimal (base 10) string.
	// Returns a new Int and panics if any error occurred.
	func MustFromDecimal(decimal string) *Int
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@holiman holiman holiman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@karalabe @holiman