Skip to content

fix(kms-connector): increase gas_limit#258

Merged
eudelins-zama merged 4 commits intomainfrom
simon/fix/connector-gas-limit
Jun 16, 2025
Merged

fix(kms-connector): increase gas_limit#258
eudelins-zama merged 4 commits intomainfrom
simon/fix/connector-gas-limit

Conversation

@eudelins-zama
Copy link
Copy Markdown
Contributor

No description provided.

@eudelins-zama eudelins-zama requested review from fd0r and maksymsur June 13, 2025 10:17
@eudelins-zama eudelins-zama force-pushed the simon/fix/connector-gas-limit branch from 0c6b9f2 to ca43778 Compare June 13, 2025 10:35
@eudelins-zama eudelins-zama marked this pull request as ready for review June 13, 2025 10:36
Comment thread kms-connector/README.md Outdated
@maksymsur
Copy link
Copy Markdown
Contributor

maksymsur commented Jun 13, 2025

I have doubts about the reasoning behind this PR. Setting a manual gas limit provides a strict upper bound, ensuring trxs do not exceed predictable cost boundaries. Currently, we automatically estimate gas based on trx complexity and size. By explicitly setting KMS_CONNECTOR_GAS_LIMIT, we’re introducing a safeguard rather than fully overriding automation. If our goal differs - like dynamically adjusting based on transaction specifics - we should explicitly derive gas limit logic based on transaction size/bytes, computational steps, or other criteria to ensure reliability and optimize costs.

@eudelins-zama eudelins-zama force-pushed the simon/fix/connector-gas-limit branch from ca43778 to fc8d3af Compare June 13, 2025 15:28
maksymsur
maksymsur approved these changes Jun 13, 2025
@eudelins-zama eudelins-zama changed the title fix(kms-connector): add gas_limit fix(kms-connector): increase gas_limit Jun 13, 2025
@eudelins-zama eudelins-zama force-pushed the simon/fix/connector-gas-limit branch 2 times, most recently from 5b33faf to d5e5947 Compare June 16, 2025 07:50
@maksymsur
Copy link
Copy Markdown
Contributor

maksymsur commented Jun 16, 2025

LGTM!
Although, can you please:

  1. Add notes (TODO) clearly marking this MAX buffer as a temporary workaround, explaining that the current automatic gas estimation logic is failing under our conditions. Also, you may reference the Slack conversation with gas consumption screenshots for context.
  2. Insert more detailed logging (TODO to replace with proper metrics later), showing clearly:
  • estimated gas before submitting the transaction,
  • call-data length,
  • actual gas consumed as per the transaction receipt, other relative data as needed.

This will allow us to gather accurate statistics and later refine our gas estimation logic based on real Arbitrum network behavior in our scenarios.

Copy link
Copy Markdown
Contributor

@maksymsur maksymsur left a comment

Choose a reason for hiding this comment

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

GREAT! Thanks!

@eudelins-zama eudelins-zama force-pushed the simon/fix/connector-gas-limit branch 4 times, most recently from 8db7bf0 to 79e6beb Compare June 16, 2025 14:31
@eudelins-zama eudelins-zama force-pushed the simon/fix/connector-gas-limit branch from 79e6beb to 77003c9 Compare June 16, 2025 14:59
@eudelins-zama eudelins-zama merged commit 5a2e329 into main Jun 16, 2025
54 checks passed
@eudelins-zama eudelins-zama deleted the simon/fix/connector-gas-limit branch June 16, 2025 15:04
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.

2 participants