Configurable teleporter-max-gas-limit #621
Conversation
destination_client factory fails fast when creating a client for C-Chain with too high teleporter-max-gas-limit
…ch configured destination_client instead of hardcoded const
najeal
requested review from
iansuvak,
richardpringle,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
vms/evm/destination_client.go
Outdated
| destinationID, err := ids.FromString(destinationBlockchain.BlockchainID) | ||
| maxGasLimit := destinationBlockchain.TeleporterMaxGasLimit | ||
| isCChain := destinationID == cChainID | ||
| if isCChain && maxGasLimit > config.DefaultTeleporterMaxGasLimit { |
There was a problem hiding this comment.
Can we do this validation in DestinationBlockchain.Validate instead? If we do so, there shouldn't be any need to pass in cChainID as a parameter.
vms/evm/destination_client.go
Outdated
| @@ -210,3 +226,7 @@ func (c *destinationClient) SenderAddress() common.Address { | |||
| func (c *destinationClient) DestinationBlockchainID() ids.ID { | |||
| return c.destinationBlockchainID | |||
| } | |||
|
|
|||
| func (c *destinationClient) TeleporterMaxGasLimit() uint64 { | |||
There was a problem hiding this comment.
Let's rename TeleporterMaxGasLimit to BlockGasLimit here and elsewhere. The destination client tx construction limits are not exclusive to Teleporter.
| const ( | ||
| // The maximum gas limit that can be specified for a Teleporter message | ||
| // Based on the C-Chain 15_000_000 gas limit per block, with other Warp message gas overhead conservatively estimated. | ||
| DefaultTeleporterMaxGasLimit = 12_000_000 |
There was a problem hiding this comment.
Once the validation is moved to this package as I suggested in another comment, let's not export this const.
There was a problem hiding this comment.
Do we really want to make it unexported ?
We use it in two places :
-
It would require to switch these with hardcoded values.
There was a problem hiding this comment.
That unit test doesn't need the value of DefaultTeleporterMaxGasLimit specifically. Instead, we can define a gas limit const in the test itself.
Not a huge deal either way, but I'd prefer to not export application variables for unit tests as much as possible.
There was a problem hiding this comment.
Makes sense, I will make the change 👍
vms/destination_client.go
Outdated
| infoClient, err := peers.NewInfoAPI(relayerConfig.InfoAPI) | ||
| if err != nil { | ||
| logger.Error("Failed to create info API", zap.Error(err)) | ||
| return nil, err | ||
| } | ||
| cChainID, err := infoClient.GetBlockchainID(context.Background(), "C") | ||
| if err != nil { | ||
| logger.Error("Failed to get C-Chain ID", zap.Error(err)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
We shouldn't need this either if we move the C-Chain gas limit validation to the config package, as I suggested in another comment.
Why this should be merged
It closes #491
How this works
DestinationClientnow embeds configuredteleporter-max-gas-limit,messageHandlercan now decide to reject or not the message based on theteleporter-max-gas-limitvalue coming from eachDestinationClientinstead of using default hardcoded value.If the new config field is not provided, previous default value is set to stay consistent with older versions of
icm-servicesHow this was tested
Existing tests have been updated.
How is this documented