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

Staking withdrawals page #9248

Merged
merged 61 commits into from
Feb 7, 2023
Merged

Staking withdrawals page #9248

merged 61 commits into from
Feb 7, 2023

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Jan 20, 2023

Description

MVP for new staking withdrawals page to live at /staking/withdrawals

  • Adds a widget for users to quickly check their validator withdrawal credentials
  • Explains who is affected that may need to act
  • Explains how staking withdrawals will function, keeping explanations higher level
  • Discussed Shanghai/Capella and timing
  • Provides resources for further reading

Information related to how validator operators actually update their credentials is out-of-scope for this page, and reserved for the Launchpad and peripheral resources.
More technical explanations of how EIP-4895 works will be reserved for future updates in the /developers/docs/ section of the site.

Preview link

https://ethereumorgwebsitedev01-stakingwithdrawals.gatsbyjs.io/en/staking/withdrawals

Related Issue

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits review needed 👀 labels Jan 20, 2023
@ethereum ethereum deleted a comment from gatsby-cloud bot Jan 23, 2023
per figma design iteration and sync discussion
move mainnet/testnet toggle above index input. remove colored styling when toggled to the right. fixed default index of 0. added typing for handler.
Staking withdrawals will be enabled through the Shanghai/Capella upgrade. This upgrade is expected to take place in the first half of 2023. <a href="#when">More below</a>
</UpgradeStatus>

The Shanghai/Capella upgrade enables **staking withdrawals** on Ethereum, allowing people to unlock rewards and optionally fully withdrawal funds from staking. Rewards payments will automatically and regularly be sent to a provided withdrawal address linked to each validator. Users can also exit staking entirely, unlocking their full validator balance.
Copy link
Contributor

@0xMNG 0xMNG Jan 23, 2023

Choose a reason for hiding this comment

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

Hi, I am a staker/front-end eng and would like to contribute. I'd like to start with documentation/copy. Let me know if I can help!

The Shanghai/Capella Ethereum upgrade will enable staking withdrawals. Staking withdrawals will allow validators to fully withdraw their deposited funds or withdraw the excess balance above 32 ETH.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @0xMNG! Thanks for hoping in to help out.. I'll take a look through and see what we can incorporate.

Think we can use part of this in the next iteration, but we were looking to trim out the comment about full withdrawals towards the beginning since this is mentioned explicitly at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Left a couple comments, pulled one suggestion in... a handful of them I felt were more semantic adjustments and wasn't sure they were adding too much, but I'm biased =) so I'll leave them for others to comment. Thank you for helping out!

More adjustments will continue to roll out here

Switch input to "string" type, allowing use of placeholder text, removing need for label. moves testnet toggle above input. adds flex-wrap. Hides long format withdrawal address on mobile.
remove custom card component, replace with existing Card component
two button approach, with condensed response and simplified copy function
@samajammin
Copy link
Member

Design comment - I don't LOVE having the navigation for this page sit under the "staking options" dropdown... but I think it's good for a short-term solution (doesn't require any design changes):
Image 2023-01-24 at 4 45 36 PM

summaryPoints:
- The Shanghai upgrade brings staking withdrawals to Ethereum
- Validator operators must provide a withdrawal address to enable
- Rewards payments are automatically sent to the address provided every few days
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Rewards payments are automatically sent to the address provided every few days
- Reward payments are automatically sent to the address provided every few days

I think just "reward payments"? The double plural feels weird (but I'm no grammar expert)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Rewards payments are automatically sent to the address provided every few days
- Rewards are automatically sent to the address provided every few days

Maybe the "payments" is not needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that double plural is weird to me too, also agree that payments maybe arent needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Advocating for "payments" as it distinguishes between just your earned rewards, and the act of moving those rewards in the form of a payment.

Given that these are "rewards", not "a single reward", and that we are talking about the payouts in a pluralistic way, as in "payments", that's why "rewards payments" was chosen here... But I get it... "Reward payments" is fine with me if people think it's too awkward

wackerow and others added 4 commits February 6, 2023 14:29
TableContainer that start-aligns table on desktop, and centers on mobile, fitting the width to the content, and adds a 1rem gap to columns
Swapped Launchpad links to use Zhejiang branch since that is currently the only link that is functional. Will update when that page migrates to mainnet version of site.
Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

Awesome work @wackerow! This is great.

I left a couple of comments I had while going through this.

@ethereum ethereum deleted a comment Feb 7, 2023
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Wondering why we don't see the breadcrumbs for this page 🤔 I think we need to add it to the intl/en.json file first, right?

image

const CopyToClipboard: React.FC<IProps> = ({
children,
text,
inline = false,
Copy link
Member

Choose a reason for hiding this comment

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

Lets try to minimize the amount of props to keep the component as simple as possible. What do you think of the following idea:

// 1. extend the props
interface IProps extends ChakraProps {
  ...
}

// 2. spread the rest of the props in the parent element
<Box ref={targetEl} cursor="pointer" {...props}>

Now from the consumer side you could use chakra props to do what you want to do

<CopyToClipboard display="inline">

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure... didn't want to creep into other components too much but I can fix this one up

src/components/Staking/ShanghaiCapella.tsx Outdated Show resolved Hide resolved
The majority of stakers did not provide a withdrawal address on
initial deposit, and will need to update their withdrawal
credentials. The{" "}
<Link href="https://zhejiang.launchpad.ethereum.org/withdrawals">
Copy link
Member

Choose a reason for hiding this comment

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

Lets use our internal Link component src/components/Link. This link should be external and open in a new tab.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good catch, that was my intent. Will patch

Copy link
Member Author

Choose a reason for hiding this comment

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

Do me a favor and check your cache... this is working for me (arrow and opening in a new tab). Our Link component parses for this automatically so we shouldn't need this flag.


Withdrawal functionality will be enabled through a two-part simultaneous network upgrade, **Shanghai + Capella**.

<ShanghaiCapella />
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if somehow we could write the content in the md file and accept it as its children 🤔 for translation concerns.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above, I agree. Let's scope that to a subsequent initiative if you're okay with that.

>
<Button
onClick={() => checkWithdrawalCredentials()}
disabled={!inputValue.length}
Copy link
Member

Choose a reason for hiding this comment

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

Since we have an isLoading flag, perhaps we could use this pattern https://chakra-ui.com/docs/components/button#button-loading-state instead of disabling the button.

<Button isLoading={isLoading}>

Copy link
Member Author

Choose a reason for hiding this comment

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

@pettinarip Not sure I understand... The disabled state is when no number is entered, not for when it's loading. Or are you referring to replacing the existing loading spinner with a loader inside the button?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the spinner below the input and replaced with isLoading on the button... I think this looked better. Kept the disabled state though when no number inputted.

Copy link
Member

Choose a reason for hiding this comment

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

Oh got it. Yeah, I thought you were disabling it when it was loading. Didn't realize that you had that other behavior.


### How do I prepare? {#how-do-i-prepare}

<WithdrawalsTabComparison />
Copy link
Member

Choose a reason for hiding this comment

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

Same concern as before related to the ease of translation management.
It's not a blocker, but I think it's worth taking the time to consider later how to pass the content to this component from the .md file, instead of having the text living inside the component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. Plan is to publish this and then tee it up for translation. When we do that, we should look at this component and consider ways to improve it for this purpose.

wackerow and others added 4 commits February 7, 2023 20:32
Co-authored-by: Corwin Smith <[email protected]>
Co-authored-by: Pablo Pettinari <[email protected]>
expand isLoading logic to designate which button was triggered
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

lgtm! great content @wackerow 👏🏼

Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

🚀

@corwintines corwintines merged commit 4af8357 into dev Feb 7, 2023
@corwintines corwintines deleted the staking-withdrawals branch February 7, 2023 21:09
@corwintines corwintines mentioned this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants