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

withdrawals, consolidations: return fee instead of excess requests from read operation #33

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Oct 29, 2024

I'm proposing we change the contract to return the required fee instead of returning the excess requests count.
The contract now computes the fee first, and then goes on to check for calldata.

The reason is simple: the read operation is a part of the contract so that people can compute the exact required
fee that must be passed. The fee computation is not straightforward, using 'fake exponential' and some constants. Any wrapper contract would have to recompute the fee from the excess value in the same way as the system contract does. Returning the fee value directly makes the formula an implementation detail, and simplifies the wrapper:

function addWithdrawal(bytes memory pubkey, uint64 amount) private {
    assert(pubkey.length == 48);

    // Read current fee from the contract.
    (bool readOK, bytes memory feeData) = WithdrawalsContract.call('');
    if (!readOK) {
        revert('reading fee failed');
    }
    uint256 fee = uint256(bytes32(feeData));

    // Add the request.
    (bool writeOK,) = WithdrawalsContract.call{value: fee}(abi.encodePacked(pubkey, amount));
    if (!writeOK) {
        revert('adding request failed');
    }
}

Note this partially undoes the changes in #30 where I moved the read operation below write. After staring at it for a while, it seems better to have the read path first, especially since it is now so small.

@mkalinin
Copy link
Contributor

My main question is the cost of the fee return, won’t it be an order of magnitude higher than the potential refund?

@fjl
Copy link
Contributor Author

fjl commented Oct 29, 2024

Note I'm just opening this PR to have a basis for discussion. We need to evaluate.

@fjl fjl changed the title WIP: withdrawals: return extra fee withdrawals, consolidations: return fee instead of excess requests from read operation Oct 29, 2024
@fjl
Copy link
Contributor Author

fjl commented Oct 29, 2024

This has reached its final form now.

fjl added a commit to fjl/EIPs that referenced this pull request Nov 7, 2024
The get operation is meant to be used by contracts to compute the exact amount of ether
required to add a withdrawal request. It does not return the fee directly though, but it
returns the count of 'excess requests' instead. The caller has to compute the fee
themselves by applying the fee formula.

I think this is not great. The fee logic, while reasonably straightforward, is an
implementation detail of the contract. Duplicating it into caller contracts could lead to
a mismatch in the computed values, and it's not necessary. I propose we change the
system contract to return the fee directly. This contract change has also been submitted
in this PR: lightclient/sys-asm#33

The Rationale section of the EIP also had some outdated text about returning fee overage
to the caller. The contract does not return overage, so I am removing that section here,
and adding recommendations & example code for calling the contract.
fjl added a commit to fjl/EIPs that referenced this pull request Nov 7, 2024
The get operation is meant to be used by contracts to compute the exact amount of ether
required to add a withdrawal request. It does not return the fee directly though, but it
returns the count of 'excess requests' instead. The caller has to compute the fee
themselves by applying the fee formula.

I think this is not great. The fee logic, while reasonably straightforward, is an
implementation detail of the contract. Duplicating it into caller contracts could lead to
a mismatch in the computed values, and it's not necessary. I propose we change the
system contract to return the fee directly. This contract change has also been submitted
in this PR: lightclient/sys-asm#33

The Rationale section of the EIP also had some outdated text about returning fee overage
to the caller. The contract does not return overage, so I am removing that section here,
and adding recommendations & example code for calling the contract.
fjl added a commit to fjl/EIPs that referenced this pull request Nov 7, 2024
The get operation is meant to be used by contracts to compute the exact amount of ether
required to add a request. It does not return the fee directly though, but it
returns the count of 'excess requests' instead. The caller has to compute the fee
themselves by applying the fee formula.

I think this is not great. The fee logic, while reasonably straightforward, is an
implementation detail of the contract. Duplicating it into caller contracts could lead to
a mismatch in the computed values, and it's not necessary. I propose we change the
system contract to return the fee directly. This contract change has also been submitted
in this PR: lightclient/sys-asm#33

The Rationale section of the EIP also had some outdated text about returning fee overage
to the caller. The contract does not return overage, so I am removing that section here,
and adding recommendations & example code for calling the contract.
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