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

support eth transfer to proposers with code #251

Merged
merged 3 commits into from
May 8, 2024

Conversation

rodrigoherrerai
Copy link
Contributor

implements 248

  • Checks if the proposer fee recipient has code, if it does: it hardcodes 250_000 gas, if it doesn't it uses the base gas (21_000).

For the near future would it be better to go with option 3? (simulate the call and based on that use the appropriate gas)

@ralexstokes
Copy link
Owner

For the near future would it be better to go with option 3? (simulate the call and based on that use the appropriate gas)

possibly, up to some gas limit, as otherwise a proposer could try to grief this builder implementation

and if we do have some fixed gas limit, it starts to look suspiciously like option 2, so not sure the extra complexity is worth it

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

this is amazing, great work!

left some suggestions


let proposer_fee_recipient_account = db.load_cache_account(config.proposer_fee_recipient)?;
let is_empty_code_hash =
proposer_fee_recipient_account.account_info().unwrap_or_default().is_empty_code_hash();
Copy link
Owner

Choose a reason for hiding this comment

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

this is very minor, but I would guess it is cheaper to unwrap at the end

as written, it would construct a full AccountInfo::default() instance, and then call is_empty_code_hash

if we do this, it would skip the instantiation if the account is missing

Suggested change
proposer_fee_recipient_account.account_info().unwrap_or_default().is_empty_code_hash();
proposer_fee_recipient_account.account_info().map(|account| account.is_empty_code_hash()).unwrap_or_default();


// If the proposer (fee recipient) has associated code, we hardcode 250_000 for gas payment.
// If it is a regular EOA, we send the base gas 21_000.
let gas_limit = if is_empty_code_hash { BASE_TX_GAS_LIMIT } else { 250_000 };
Copy link
Owner

Choose a reason for hiding this comment

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

would suggest putting another constant next to BASE_TX_GAS_LIMIT above

Suggested change
let gas_limit = if is_empty_code_hash { BASE_TX_GAS_LIMIT } else { 250_000 };
const PAYMENT_TO_CONTRACT_GAS_LIMIT: u64 = 250_000;
// ...
let gas_limit = if is_empty_code_hash { BASE_TX_GAS_LIMIT } else { PAYMENT_TO_CONTRACT_GAS_LIMIT };

Comment on lines 104 to 105
// If the proposer (fee recipient) has associated code, we hardcode 250_000 for gas payment.
// If it is a regular EOA, we send the base gas 21_000.
Copy link
Owner

Choose a reason for hiding this comment

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

and to reduce future churn, we can be more abstract in the comment, something like:

Suggested change
// If the proposer (fee recipient) has associated code, we hardcode 250_000 for gas payment.
// If it is a regular EOA, we send the base gas 21_000.
// Use a fixed gas limit for the payment transaction reflecting the recipient's status as smart contract or EOA.

@ralexstokes
Copy link
Owner

here's a lido payment: https://etherscan.io/tx/0x252940cec2d0f1d0340a5438f7abcf77f926c26931e5a53e03c10ca3aecd7981

only 22,111 gas

stakefish:
https://etherscan.io/tx/0xc9b86901e3155d36f14430779799380e4d43dd4ab50eb9d2ab388ecd9ca2e3a1
21,033 gas

rocket pool smoothing pool:
https://etherscan.io/tx/0x153a76aaefeaf09ccaf3b67f3523c5d475c9775bc1fed405247726b4fb7f2031
21,055 gas

so maybe 250k is much too high.... perhaps we just go with 100k for now

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

amazing thank you!

@ralexstokes ralexstokes merged commit f1af6d4 into ralexstokes:main May 8, 2024
4 checks passed
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