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 amount parameter #326

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

Conversation

0xCypherr
Copy link

Previously, the ether amount had to be fixed at 32 ether. In other words, the script generates keystores that signed to deposit 32 ether per validator. However, people might need to deposit any amount of ether between 1 to 32. For example, in Rocketpool protocol, operators have to deposit 16 ether. In Geode Finance, operators have to deposit 1 and 31 ether respectively. To facilitate such operations, the amount variable can be parameterized.

Changes:

  • New function validate_ether_amount_range has been created to test given input is between 1-32 and integer.
  • The scripts tests 'test_existing_mnemonics.py' and 'test_new_mnemonic.py' have been updated with '--amount 32'. Added new test to test_validation.py named 'test_validate_ether_amount_range'. CLI test remains untouched due to no prompting.
  • The amount parameter explanation added to README file.

0xCypherr added 6 commits February 9, 2023 15:02
… per validator. Intentionally, it will not be inputted in prompts of CLI and default is 32.
…emonic.py' have been updated with '--amount 32'. Added new test to test_validation.py named 'test_validate_ether_amount_range'. CLI test remains untouched due to no prompting.
@0xCypherr 0xCypherr closed this Feb 9, 2023
@0xCypherr 0xCypherr reopened this Feb 9, 2023
@geode-main
Copy link

@CarlBeek, can I get your comment on this improvement?
I am keen to see it live 🙏

Copy link

@geode-main geode-main left a comment

Choose a reason for hiding this comment

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

  • I don't see any valid reason to deposit less than 1 ether, or more than 32 ether to the validator. However, I also don't see why the limitation is here. Might be useful to remove these limitations for future usage.
  • Further testing might be needed.

@geode-main
Copy link

@ralexstokes @CarlBeek

Hey guys, we would really appreciate any feedback on this feature.

  • Do you think it doesn't make sense?
  • Do you think the implementation is unsufficient or faulty?
  • Should we discuss the boundaries (currently 1-32 ether) ?
  • Is there more testing needed?

I think this is very useful for Liquid Staking Providers and I am keen to see it live.
Thanks!

@yorickdowne
Copy link
Contributor

@geode-main could you come to eth-educators#53 please and discuss this with us. I’d like to understand the workflow and where the deposit tool comes in.

RocketPool: Their software takes care of deposits and the right amount

Geode: Haven’t looked at it yet but I expect a similar flow? Or do they use ethdo currently?

We do have a partial deposit command now for top ups, though it will require good testing on Holesky.

@geode-main
Copy link

@yorickdowne
We have tried to integrate the cli tool to our node operator infra but realized ethdo is the way to go.

It seems to me that partial-deposit command solves this, tho I did not deep dive into the solution.
Thus, this PR is ancient and we may consider it solved.

Looking forward to further discussion, let me know.

@yorickdowne
Copy link
Contributor

@geode-main Yes I’d like to understand your use case better and see whether partial-deposit really solves it. Can you find me on Discord or TG? @yorickdowne on both, on Discord it’s the ethstaker server

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