Update LimitDefault and LimitMax type from uint32 to uint64 #12307
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
|
|
|
||
| if result > math.MaxUint32 { | ||
| if result.GreaterThan(decimal.NewFromBigInt(big.NewInt(0).SetUint64(math.MaxUint64), 0)) { |
There was a problem hiding this comment.
is there a reason that result > math.MaxUint64 doesnt work here?
There was a problem hiding this comment.
oh i see the IntPart() was removed
There was a problem hiding this comment.
Right and I can't compare to IntPart() since that returns int64
| @@ -561,7 +561,7 @@ func (b *Txm[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) SendNative | |||
| ToAddress: to, | |||
| EncodedPayload: []byte{}, | |||
| Value: value, | |||
| FeeLimit: gasLimit, | |||
There was a problem hiding this comment.
would it make sense to change the function signature to expect a uint64 instead of uint32 here?
My thoughts are that we might have some callers that use LimitDefault.
There was a problem hiding this comment.
I think the only place using this method uses LimitTransfer for the gas limit which is uint32. I have an open question if I should be updating it's type or not. If the answer is yes, I'll be updating this for sure. If it's no, I can still update the signature and add the cast upstream.
There was a problem hiding this comment.
Do other repos use this method?
There was a problem hiding this comment.
That's a good question. Not sure actually. This also reminds me that CCIP has its own repo where LimitDefault would still be uint32.
There was a problem hiding this comment.
I think we need not worry how other repos use Core's code.
They likely merge latest Core into their own repo regularly, and take in all changes on their schedule.
| @@ -188,9 +188,9 @@ func NewFromJobSpec( | |||
| gasLimit := fcfg.LimitDefault() | |||
| fmLimit := fcfg.LimitJobType().FM() | |||
| if jobSpec.GasLimit.Valid { | |||
| gasLimit = jobSpec.GasLimit.Uint32 | |||
| gasLimit = uint64(jobSpec.GasLimit.Uint32) | |||
There was a problem hiding this comment.
would the jobSpec.GasLimit ever need to be uint64 ?
There was a problem hiding this comment.
Potentially but I left as many types as I could in services as uint32 so it would be up to the product team if they'd like to up it.
There was a problem hiding this comment.
I think we are good, as this jobs pipeline code is deprecated, and we are trying to get rid of this
There was a problem hiding this comment.
wow this is alot of work nice job Amit... I think the PR looks good. We shall see in the future what other values might make sense to have as uint64 instead of uint32, but i think this is a good start. Not sure how the other repos will need to coordinate these changes
Updated LimitDefault and LimitMax config type from uint32 to uint64 to support higher fee limits for transactions. Geth accept uint64
Gasvalues for transactions so this aligns our types.