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

W37 upstream merge #396

Merged
merged 64 commits into from
Oct 7, 2021
Merged

W37 upstream merge #396

merged 64 commits into from
Oct 7, 2021

Conversation

carterqw2
Copy link

@carterqw2 carterqw2 commented Sep 14, 2021

W37 upstream changes:

  • #4625 - Contract address page: Add implementation link to the overview of proxy contracts
  • #4624 - Support HTML tags in alert message
  • #4608, #4622 - Block Details page: Improved style of transactions button
  • #4596 - Display token icon for bridged with Mainnet tokens or identicons for other tokens
  • #4520 - Add support for EIP-1559
  • #4593 - Add status in Position pane for txs have no block
  • #4579 - Write contract page: Resize inputs; Improve multiplier selector
  • #4612 - Hide error selector in the contract's functions list
  • #4615 - Fix broken style for View more transfers button
  • #4592 - Add type field for receive and fallback entities of a Smart Contract
  • #4601 - Fix endless Fetching tokens... message on empty addresses
  • #4591 - Add step and min value for txValue input field
  • #4589 - Fix solid outputs on contract read page
  • #4586 - Fix floating tooltips on the token transfer family blocks
  • #4587 - Enable navbar menu on Search results page
  • #4582 - Fix NaN input on write contract page
  • #4611 - Ability to hide miner in block views

Mostly CSS fixes, a bigger one is EIP-1559 support but since we have a custom implementation, incompatible with Ethereum, more details here, it's not reflected in the UI, kept it there for compatibility.

nikitosing and others added 30 commits August 30, 2021 09:10
…rite-cntrct

Fix NaN input in write contract
…enu-fix

Enable navbar menu on Search results page
…oltips

Fix floating tooltips on token transfer block
…s-write-contract

Write contract page: Resize inputs; Improve multiplier selector
…n-args

Add step and min value for txValue input
…act-outputs

Fix solid outputs on contract read page
…ching-tokens

Fix endless Fetching tokens... message on empty addresses
…ion-names

Fix empty contract's function names
@github-actions
Copy link

github-actions bot commented Sep 15, 2021

Unit Test Results

       3 files  ±0     222 suites  ±0   3m 44s ⏱️ +27s
2 050 tests ±0  2 005 ✔️ ±0  45 💤 ±0  0 ±0 

Results for commit 49436ab. ± Comparison against base commit 816e0e1.

♻️ This comment has been updated with latest results.

@carterqw2 carterqw2 marked this pull request as ready for review September 22, 2021 10:09
@carterqw2 carterqw2 requested a review from a team September 22, 2021 10:09
Copy link

@eruizgar91 eruizgar91 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -38,7 +38,7 @@ jobs:
path: |
deps
_build
key: ${{ runner.os }}-${{ env.ELIXIR_VERSION }}-${{ env.OTP_VERSION }}-${{ env.MIX_ENV }}-deps-mixlockhash-${{ hashFiles('mix.lock') }}
key: ${{ runner.os }}-${{ env.ELIXIR_VERSION }}-${{ env.OTP_VERSION }}-${{ env.MIX_ENV }}-deps-mixlockhash_1-${{ hashFiles('mix.lock') }}

Choose a reason for hiding this comment

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

weird change to the hash key but ok

break
}
if (chainName) {
return `https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/${chainName}/assets/${addressHash}/logo.png`

Choose a reason for hiding this comment

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

It feels weird to host assets on github

@@ -10,6 +10,16 @@
<div class="centered-container">
<div>
<div style="display: table;">
<%= if System.get_env("DISPLAY_TOKEN_ICONS") === "true" do %>
<% foreign_chain_id = if Map.has_key?(@bridged_token, :foreign_chain_id), do: @bridged_token.foreign_chain_id, else: nil %>
<% chain_id_for_token_icon = if foreign_chain_id, do: foreign_chain_id |> Decimal.to_integer() |> to_string(), else: System.get_env("CHAIN_ID") %>

Choose a reason for hiding this comment

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

I guess this is where the github icon js is getting it's input from. In future we could do something similar but serve icons from the app. I don't know enough about bridged tokens or if our implementation of them can be detected by blockscout in the same way as Ethereum

Copy link

@rkachowski rkachowski left a comment

Choose a reason for hiding this comment

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

Seems mostly ok. Some minor eth -> celo things. It would be nice to have this on staging to smoke test the changes


<%= case function["type"] do %>
<% "fallback" -> %>
<%= gettext "fallback" %><%= render BlockScoutWeb.CommonComponentsView, "_i_tooltip_2.html", text: gettext("The fallback function is executed on a call to the contract if none of the other functions match the given function signature, or if no data was supplied at all and there is no receive Ether function. The fallback function always receives data, but in order to also receive Ether it must be marked payable.") %>

Choose a reason for hiding this comment

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

Ether needs to be changed to Celo or other token that can be used here

<% "fallback" -> %>
<%= gettext "fallback" %><%= render BlockScoutWeb.CommonComponentsView, "_i_tooltip_2.html", text: gettext("The fallback function is executed on a call to the contract if none of the other functions match the given function signature, or if no data was supplied at all and there is no receive Ether function. The fallback function always receives data, but in order to also receive Ether it must be marked payable.") %>
<% "receive" -> %>
<%= gettext "receive" %><%= render BlockScoutWeb.CommonComponentsView, "_i_tooltip_2.html", text: gettext("The receive function is executed on a call to the contract with empty calldata. This is the function that is executed on plain Ether transfers (e.g. via .send() or .transfer()). If no such function exists, but a payable fallback function exists, the fallback function will be called on a plain Ether transfer. If neither a receive Ether nor a payable fallback function is present, the contract cannot receive Ether through regular transactions and throws an exception.") %>

Choose a reason for hiding this comment

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

More ether

<% end %>
</div>
<% end %>
<% end %>

<%= if Helper.payable?(function) do %>
<div class="form-group pr-3">
<input type="text" name="function_input" tx-value class="form-control form-control-sm address-input-sm mt-2" placeholder='value(<%= gettext("ETH")%>)' />
<input type="number" name="function_input" tx-value
data-toggle="tooltip" title='Amount in native token <<%= gettext("ETH")%>>' class="form-control form-control-sm address-input-sm mt-2" placeholder='value(<%= gettext("ETH")%>)' min="0" step="1e-18" />

Choose a reason for hiding this comment

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

ETH to Celo

@@ -6,6 +6,7 @@ defmodule BlockScoutWeb.BlockView do
alias Explorer.Chain
alias Explorer.Chain.{Address, Block, Wei}
alias Explorer.Chain.Block.Reward
alias Explorer.Counters.{BlockBurnedFeeCounter, BlockPriorityFeeCounter}

Choose a reason for hiding this comment

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

Weird that this is added without being used

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that's because they are used in the template.


if chain_name do
try_url =
"https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/#{chain_name}/assets/#{address_hash}/logo.png"

Choose a reason for hiding this comment

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

still weird

@rkachowski
Copy link

I don't know how github differentiates between review comments and normal code comments. Just ignore anything that says "weird"

@eruizgar91 eruizgar91 merged commit 5900c7c into master Oct 7, 2021
@eruizgar91 eruizgar91 deleted the carterqw2/w37-upstream-merge branch October 7, 2021 14:08
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.

None yet

5 participants