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: reduce required eth #2018

Merged
merged 1 commit into from
Jun 7, 2021
Merged

feat: reduce required eth #2018

merged 1 commit into from
Jun 7, 2021

Conversation

ralph-pichler
Copy link
Member

@ralph-pichler ralph-pichler commented Jun 7, 2021

reduce required eth for initial deployment. value was still based on the pre proxy factory.

This change is Reviewable

@ralph-pichler ralph-pichler added the ready for review The PR is ready to be reviewed label Jun 7, 2021
@ralph-pichler ralph-pichler self-assigned this Jun 7, 2021
@ralph-pichler ralph-pichler merged commit 1890f32 into master Jun 7, 2021
@ralph-pichler ralph-pichler deleted the reduce_eth branch June 7, 2021 15:15
@Eknir
Copy link
Contributor

Eknir commented Jun 7, 2021

Can't we do a minimum of 0? Before, this was required because otherwise the node wouldn't function. Now, it is completely up to the user.

@ldeffenb
Copy link
Collaborator

ldeffenb commented Jun 7, 2021

Can't we do a minimum of 0? Before, this was required because otherwise the node wouldn't function. Now, it is completely up to the user.

But wouldn't a minimum of zero cause bee to queue the checkbook deployment transaction even if there was zero gETH available in the account? And that would just cause the transaction to fail over and over again.

Checkbook contract deployment does have a known gas cost, and I would say that it is reasonable for the bee node to ensure that there is at least that much gETH available to pay for it without sending a transaction that is guaranteed to fail.

But that's only my $0.02 (2 cents) and you can take it for whatever it's worth.

@ldeffenb
Copy link
Collaborator

ldeffenb commented Jun 7, 2021

And isn't "up to the user" actually running a non-full-node? Which shouldn't try to deploy the checkbook anyway. But in a full-node: true, the checkbook is still required, isn't it?

@ralph-pichler
Copy link
Member Author

@Eknir if they want to run with 0, they can start with swap-enable: false which skips this check entirely. (downside is that if they are a full node and somebody wants to send them a cheque they will get blocklisted)

@cppfuns
Copy link

cppfuns commented Jun 15, 2021

Wouldn't it be better to manage this value through configuration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants