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 withdrawals support (EIP-4895) #6694

Merged
merged 3 commits into from
May 4, 2023
Merged

Add withdrawals support (EIP-4895) #6694

merged 3 commits into from
May 4, 2023

Conversation

sl1depengwyn
Copy link
Collaborator

@sl1depengwyn sl1depengwyn commented Jan 10, 2023

Closes #6455

Changelog

  • Add tab withdrawal to block page
    image

  • Add tab withdrawal to address page
    image

  • Add withdrawal fetcher (disabled by default because at the moment withdrawals is in the development, so withdrawals fetcher is not needed now at the majority of the chains)

Nodes for testing: https://github.com/ethpandaops/withdrawals-testnet/blob/master/withdrawal-devnet-3/inventory/inventory.ini (if down try to check withdrawal-devnet-n+1 or public net that going to be available soon)

Checklist for your Pull Request (PR)

Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@sl1depengwyn could you please add a screenshot(s) of UI changes for support of withdrawals feature to the description of PR?

@sl1depengwyn
Copy link
Collaborator Author

@sl1depengwyn could you please add a screenshot(s) of UI changes for support of withdrawals feature to the description of PR?

Done

Copy link
Member

@nikitosing nikitosing left a comment

Choose a reason for hiding this comment

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

Nitpick: let's leave just block number: 50197 instead of Block #50197 in Block column

image

Copy link
Member

@nikitosing nikitosing left a comment

Choose a reason for hiding this comment

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

Let's add pagination buttons to the bottom of address withdrawal list

image

if reason === :normal do
{:noreply, state}
else
Logger.metadata(fetcher: :transaction_action)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy paste?

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Jan 30, 2023

Choose a reason for hiding this comment

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

Yep, I'll fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 1670e8f

select: block.number,
distinct: block.number,
where: block.number >= ^from,
where: is_nil(withdrawal.index)
Copy link
Member

Choose a reason for hiding this comment

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

consider to add here condition for block.consensus == true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 1670e8f

@sl1depengwyn sl1depengwyn force-pushed the mf-withdrawals branch 2 times, most recently from 896a520 to 1670e8f Compare February 7, 2023 19:23
@sl1depengwyn
Copy link
Collaborator Author

Let's add pagination buttons to the bottom of address withdrawal list

image

Nitpick: let's leave just block number: 50197 instead of Block #50197 in Block column

image

Done in 1670e8f

@sl1depengwyn sl1depengwyn force-pushed the mf-withdrawals branch 6 times, most recently from 9cb6ee5 to 8d753cc Compare April 12, 2023 05:06
@sl1depengwyn sl1depengwyn requested review from nikitosing and vbaranov and removed request for vbaranov April 12, 2023 05:43
@sl1depengwyn sl1depengwyn force-pushed the mf-withdrawals branch 2 times, most recently from b60f44b to fa09475 Compare April 13, 2023 09:22
@sl1depengwyn sl1depengwyn force-pushed the mf-withdrawals branch 2 times, most recently from f526128 to b8c3052 Compare April 13, 2023 09:59
@spec check_if_withdrawals_in_block(Hash.Full.t()) :: boolean()
def check_if_withdrawals_in_block(block_hash) do
block_hash
|> Withdrawal.block_hash_to_withdrawals_query()
Copy link
Member

Choose a reason for hiding this comment

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

This is really micro improvment but nevertheless. Let's use here version of this query without order_by?

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Make sense, fixed here bca0f23

@spec check_if_withdrawals_at_address(Hash.Address.t()) :: boolean()
def check_if_withdrawals_at_address(address_hash) do
address_hash
|> Withdrawal.address_hash_to_withdrawals_query()
Copy link
Member

Choose a reason for hiding this comment

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

The same as above

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Make sense, fixed here bca0f23

use BlockScoutWeb, :view

# import BlockScoutWeb.AddressView, only: [balance: 1]
# import BlockScoutWeb.Tokens.OverviewView, only: [total_supply_usd: 1]
Copy link
Member

Choose a reason for hiding this comment

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

Is it still needed?

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Nope, thanks, fixed in bca0f23

{:ok, block} <- fetch_block(type, value, @api_true) do
full_options =
[necessity_by_association: %{address: :optional}]
|> Keyword.merge(paging_options(params))
Copy link
Member

Choose a reason for hiding this comment

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

Needed to add api?: true here

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Fixed in bca0f23

with {:format, {:ok, address_hash}} <- {:format, Chain.string_to_address_hash(address_hash_string)},
{:ok, false} <- AccessHelper.restricted_access?(address_hash_string, params),
{:not_found, {:ok, _address}} <- {:not_found, Chain.hash_to_address(address_hash, @api_true, false)} do
withdrawals_plus_one = address_hash |> Chain.address_hash_to_withdrawals(paging_options(params))
Copy link
Member

Choose a reason for hiding this comment

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

Here also api?: true is missed

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Fixed in bca0f23

@@ -370,6 +370,22 @@ defmodule BlockScoutWeb.API.V2.AddressController do
end
end

def withdrawals(conn, %{"address_hash" => address_hash_string} = params) do
Copy link
Member

Choose a reason for hiding this comment

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

I think we need also add new field to address entity: has_beacon_chain_withdrawals to apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex:110 (Don't forget about api?: true)

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 same applies to the block view, am I right?

Copy link
Member

Choose a reason for hiding this comment

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

yes but only for case when renders single block

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Done in bca0f23

blocks
end

defp parse_integer(nil), do: nil
Copy link
Member

Choose a reason for hiding this comment

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

I think we can reuse parse_integer from apps/explorer/lib/explorer/helper.ex

Copy link
Collaborator Author

@sl1depengwyn sl1depengwyn Apr 13, 2023

Choose a reason for hiding this comment

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

Done in bca0f23

Add withdrawals to explorer app

Add withdrawals to indexer

Add withdrawal tabs to address and block pages

Add withdrawals test to web

Add withdrawals test to eth json rpc

Add withdrawals test to explorer

Add withdrawal test to indexer

Update CHANGELOG.md

Update gettext

[no ci]

Update apps/indexer/config/prod.exs

[no ci]

Co-authored-by: nikitosing <32202610+nikitosing@users.noreply.github.com>

Fix review

Update CHANGELOG.md

Update CHANGELOG.md

Add withdrawal list page; Add withdrawal to APIv2

Add timestamp

Fix nikitosing review
@sl1depengwyn sl1depengwyn force-pushed the mf-withdrawals branch 3 times, most recently from a81018a to ded0cd5 Compare May 3, 2023 17:51
@vbaranov vbaranov merged commit 9615e2f into master May 4, 2023
1 check passed
@vbaranov vbaranov deleted the mf-withdrawals branch May 4, 2023 08:52
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.

Support EIP 4895 (POS withdrawals)
4 participants