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

Re-work REACT_APP_MIN_DEPOSIT_CLI_VERSION usage #719

Open
CarlBeek opened this issue Dec 19, 2024 · 1 comment
Open

Re-work REACT_APP_MIN_DEPOSIT_CLI_VERSION usage #719

CarlBeek opened this issue Dec 19, 2024 · 1 comment
Milestone

Comments

@CarlBeek
Copy link
Contributor

Now that there are more audited versions of the staking-deposit-cli, it is time to rethink how we do ensure that the deposit_data.json was generated by a tool with sufficient features.

Currently, we use the REACT_APP_MIN_DEPOSIT_CLI_VERSION constant to ensure a minimum feature set, but this is awkwardly tied to the https://github.com/ethereum/staking-deposit-cli which noi longer seems appropriate.

One approach would be to have text-based features we add to ensure a minimum level of support and check for those (eg. testnet-holesky, bug-fix-123), but this feels a little over engineered for our current needs.

Personally I am partial to sticking with the numbering approach, but just agreeing with the ethstaker folks as to when we need to bump the version number. We could even maintain a doc in this repo that maps features to version numbers.

cc @remyroy @valefar-on-discord I'd love to hear your thoughts here.

@CarlBeek CarlBeek added this to the Pectra Update milestone Dec 19, 2024
@valefar-on-discord
Copy link
Contributor

Thoughts:

  • EthStaker and other potential tools can have a unique field that would define the necessary version for that tool. IE instead of using deposit_cli_version we could use ethstaker_cli_version. Updating DepositKeyInterface and then making use of that optional field would be a pretty trivial change and easy to maintain for future versioning updates.

  • Similar to what you suggested we can add a field that defines an array of feature flags that can be utilized in the launchpad. I do agree with you though that this seems a bit much for where we are now

  • We can always discuss the potential of removing this check entirely given the ease of spoofing.

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

No branches or pull requests

2 participants