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

Add methods for batch withdrawals requests and claiming #513

Merged

Conversation

Psirex
Copy link
Contributor

@Psirex Psirex commented Jan 29, 2023

No description provided.

/// @notice Request withdrawal of the provided token amount in a batch
/// NB: Always reverts
function requestWithdrawalBatch(uint256[] calldata, address[] calldata)
struct WithdrawalRequestInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice idea 👍

Copy link
Member

Choose a reason for hiding this comment

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

Is there any drawbacks in reusing these structs in all the methods?

Copy link
Member

Choose a reason for hiding this comment

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

Or we can even get rid of the singular methods at all. I like this idea more and more.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DiRaiks fyi

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

Thank you! 👍
Left some considerations 👀

contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
Base automatically changed from feature/wq_review_fixes to feature/shapella-upgrade January 30, 2023 12:14
Copy link
Member

@folkyatina folkyatina left a comment

Choose a reason for hiding this comment

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

I think we should get rid of singular request/claim in favor of plural and get rid of the Batch in the names as well. It will give a smaller surface and more consistency.

contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
/// @notice Request withdrawal of the provided token amount in a batch
/// NB: Always reverts
function requestWithdrawalBatch(uint256[] calldata, address[] calldata)
struct WithdrawalRequestInput {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any drawbacks in reusing these structs in all the methods?

contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
/// @notice Request withdrawal of the provided token amount in a batch
/// NB: Always reverts
function requestWithdrawalBatch(uint256[] calldata, address[] calldata)
struct WithdrawalRequestInput {
Copy link
Member

Choose a reason for hiding this comment

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

Or we can even get rid of the singular methods at all. I like this idea more and more.

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

LGTM mostly, left some additional suggestions

contracts/0.8.9/WithdrawalQueue.sol Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Outdated Show resolved Hide resolved
contracts/0.8.9/WithdrawalQueue.sol Show resolved Hide resolved
@folkyatina folkyatina merged commit 35b1921 into feature/shapella-upgrade Jan 31, 2023
@folkyatina folkyatina deleted the feature/wq_rewiew_fixes_batch_withdrawals branch January 31, 2023 13:25
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.

3 participants