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

Feat/safe module #107

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

Feat/safe module #107

wants to merge 22 commits into from

Conversation

samparsky
Copy link
Collaborator

Fixes #91

Copy link
Contributor

@OisinKyne OisinKyne left a comment

Choose a reason for hiding this comment

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

Want to understand whether this can break if the amount of contribution addresses goes beyond a transfer()s gas mainly

src/interfaces/IETH2DepositContract.sol Show resolved Hide resolved
depositContract = IETH2DepositContract(eth2DepositContract);
}

receive() external payable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we calculate at how many contributors might this break with 2100 gas, because the free gas in a receive is lower than the allocation of a new storage slot to write down their contribution?

We don't have to worry about fallback() when receive() is set, yes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not we don't have to worry about fallback()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The 2100 gas limit doesn't hold here. Any interaction via transfer/send will revert with OOG

src/safe-modules/SimpleETHContributionVault.sol Outdated Show resolved Hide resolved
src/safe-modules/SimpleETHContributionVault.sol Outdated Show resolved Hide resolved
handler.ghost_depositSum() - mockDepositContract.ghost_depositSum() - handler.ghost_rageQuitSum()
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand this testing much, walk me through it on our next one on one call :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, definitely

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.

deposit Ether (ETH) to the beacon chain collectively
2 participants