code review issues addressed

blockscout · May 1, 2024 · 8156d20 · varasev · May 13, 2024 · 8156d20
1 parent f411222
commit 8156d20
Show file tree

Hide file tree

Showing 21 changed files with 158 additions and 108 deletions.
diff --git a/apps/explorer/lib/explorer/chain/arbitrum/batch_block.ex b/apps/explorer/lib/explorer/chain/arbitrum/batch_block.ex
@@ -1,5 +1,13 @@
 defmodule Explorer.Chain.Arbitrum.BatchBlock do
-  @moduledoc "Models a list of blocks related to a batch for Arbitrum."
+  @moduledoc """
+    Models a list of blocks related to a batch for Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.BatchBlocks
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 
@@ -8,12 +16,12 @@ defmodule Explorer.Chain.Arbitrum.BatchBlock do
 
   @optional_attrs ~w(confirm_id)a
 
-  @required_attrs ~w(batch_number hash)a
+  @required_attrs ~w(batch_number block_hash)a
 
   @type t :: %__MODULE__{
           batch_number: non_neg_integer(),
           batch: %Ecto.Association.NotLoaded{} | L1Batch.t() | nil,
-          hash: Hash.t(),
+          block_hash: Hash.t(),
           block: %Ecto.Association.NotLoaded{} | Block.t() | nil,
           confirm_id: non_neg_integer() | nil,
           confirm_transaction: %Ecto.Association.NotLoaded{} | LifecycleTransaction.t() | nil
@@ -22,7 +30,7 @@ defmodule Explorer.Chain.Arbitrum.BatchBlock do
   @primary_key false
   schema "arbitrum_batch_l2_blocks" do
     belongs_to(:batch, L1Batch, foreign_key: :batch_number, references: :number, type: :integer)
-    belongs_to(:block, Block, foreign_key: :hash, primary_key: true, references: :hash, type: Hash.Full)
+    belongs_to(:block, Block, foreign_key: :block_hash, primary_key: true, references: :hash, type: Hash.Full)
 
     belongs_to(:confirm_transaction, LifecycleTransaction,
       foreign_key: :confirm_id,
@@ -43,6 +51,6 @@ defmodule Explorer.Chain.Arbitrum.BatchBlock do
     |> validate_required(@required_attrs)
     |> foreign_key_constraint(:batch_number)
     |> foreign_key_constraint(:confirm_id)
-    |> unique_constraint(:hash)
+    |> unique_constraint(:block_hash)
   end
 end
diff --git a/apps/explorer/lib/explorer/chain/arbitrum/batch_transaction.ex b/apps/explorer/lib/explorer/chain/arbitrum/batch_transaction.ex
@@ -1,27 +1,38 @@
 defmodule Explorer.Chain.Arbitrum.BatchTransaction do
-  @moduledoc "Models a list of transactions related to a batch for Arbitrum."
+  @moduledoc """
+    Models a list of transactions related to a batch for Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.BatchTransactions
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 
+  alias Explorer.Chain.Arbitrum.L1Batch
   alias Explorer.Chain.{Hash, Transaction}
-  alias Explorer.Chain.Arbitrum.{BatchBlock, L1Batch}
 
-  @required_attrs ~w(batch_number block_hash hash)a
+  @required_attrs ~w(batch_number tx_hash)a
 
   @type t :: %__MODULE__{
           batch_number: non_neg_integer(),
           batch: %Ecto.Association.NotLoaded{} | L1Batch.t() | nil,
-          block_hash: Hash.t(),
-          block: %Ecto.Association.NotLoaded{} | BatchBlock.t() | nil,
-          hash: Hash.t(),
+          tx_hash: Hash.t(),
           l2_transaction: %Ecto.Association.NotLoaded{} | Transaction.t() | nil
         }
 
   @primary_key false
   schema "arbitrum_batch_l2_transactions" do
     belongs_to(:batch, L1Batch, foreign_key: :batch_number, references: :number, type: :integer)
-    belongs_to(:block, BatchBlock, foreign_key: :block_hash, references: :hash, type: Hash.Full)
-    belongs_to(:l2_transaction, Transaction, foreign_key: :hash, primary_key: true, references: :hash, type: Hash.Full)
+
+    belongs_to(:l2_transaction, Transaction,
+      foreign_key: :tx_hash,
+      primary_key: true,
+      references: :hash,
+      type: Hash.Full
+    )
 
     timestamps()
   end
@@ -36,6 +47,6 @@ defmodule Explorer.Chain.Arbitrum.BatchTransaction do
     |> validate_required(@required_attrs)
     |> foreign_key_constraint(:batch_number)
     |> foreign_key_constraint(:block_hash)
-    |> unique_constraint(:hash)
+    |> unique_constraint(:tx_hash)
   end
 end
diff --git a/apps/explorer/lib/explorer/chain/arbitrum/l1_batch.ex b/apps/explorer/lib/explorer/chain/arbitrum/l1_batch.ex
@@ -1,5 +1,13 @@
 defmodule Explorer.Chain.Arbitrum.L1Batch do
-  @moduledoc "Models an L1 batch for Arbitrum."
+  @moduledoc """
+    Models an L1 batch for Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.L1Batches
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 

diff --git a/apps/explorer/lib/explorer/chain/arbitrum/l1_execution.ex b/apps/explorer/lib/explorer/chain/arbitrum/l1_execution.ex
@@ -1,5 +1,13 @@
 defmodule Explorer.Chain.Arbitrum.L1Execution do
-  @moduledoc "Models a list of execution transactions related to a L2 to L1 messages on Arbitrum."
+  @moduledoc """
+    Models a list of execution transactions related to a L2 to L1 messages on Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.L1Executions
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 
@@ -9,7 +17,7 @@ defmodule Explorer.Chain.Arbitrum.L1Execution do
 
   @type t :: %__MODULE__{
           message_id: non_neg_integer(),
-          execution_id: non_neg_integer() | nil,
+          execution_id: non_neg_integer(),
           execution_transaction: %Ecto.Association.NotLoaded{} | LifecycleTransaction.t() | nil
         }
 

diff --git a/apps/explorer/lib/explorer/chain/arbitrum/lifecycle_transaction.ex b/apps/explorer/lib/explorer/chain/arbitrum/lifecycle_transaction.ex
@@ -1,5 +1,13 @@
 defmodule Explorer.Chain.Arbitrum.LifecycleTransaction do
-  @moduledoc "Models an L1 lifecycle transaction for Arbitrum."
+  @moduledoc """
+    Models an L1 lifecycle transaction for Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.LifecycleTransactions
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 
@@ -10,19 +18,19 @@ defmodule Explorer.Chain.Arbitrum.LifecycleTransaction do
 
   alias Explorer.Chain.Arbitrum.{BatchBlock, L1Batch}
 
-  @required_attrs ~w(id hash block timestamp status)a
+  @required_attrs ~w(id hash block_number timestamp status)a
 
   @type t :: %__MODULE__{
           hash: Hash.t(),
-          block: Block.block_number(),
+          block_number: Block.block_number(),
           timestamp: DateTime.t(),
           status: String.t()
         }
 
   @primary_key {:id, :integer, autogenerate: false}
   schema "arbitrum_lifecycle_l1_transactions" do
     field(:hash, Hash.Full)
-    field(:block, :integer)
+    field(:block_number, :integer)
     field(:timestamp, :utc_datetime_usec)
     field(:status, Ecto.Enum, values: [:unfinalized, :finalized])
 

diff --git a/apps/explorer/lib/explorer/chain/arbitrum/message.ex b/apps/explorer/lib/explorer/chain/arbitrum/message.ex
@@ -1,5 +1,13 @@
 defmodule Explorer.Chain.Arbitrum.Message do
-  @moduledoc "Models an L1<->L2 messages on Arbitrum."
+  @moduledoc """
+    Models an L1<->L2 messages on Arbitrum.
+
+    Changes in the schema should be reflected in the bulk import module:
+    - Explorer.Chain.Import.Runner.Arbitrum.Messages
+
+    Migrations:
+    - Explorer.Repo.Arbitrum.Migrations.CreateArbitrumTables
+  """
 
   use Explorer.Schema
 

diff --git a/apps/explorer/lib/explorer/chain/arbitrum/reader.ex b/apps/explorer/lib/explorer/chain/arbitrum/reader.ex
@@ -120,10 +120,11 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       )
 
     case query
+         # :required is used since the situation when commit transaction is not found is not possible
          |> Chain.join_associations(%{:commit_transaction => :required})
          |> Repo.one() do
       nil -> nil
-      batch -> batch.commit_transaction.block
+      batch -> batch.commit_transaction.block_number
     end
   end
 
@@ -147,10 +148,11 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       )
 
     case query
+         # :required is used since the situation when commit transaction is not found is not possible
          |> Chain.join_associations(%{:commit_transaction => :required})
          |> Repo.one() do
       nil -> nil
-      batch -> batch.commit_transaction.block
+      batch -> batch.commit_transaction.block_number
     end
   end
 
@@ -215,9 +217,9 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       )
 
     query
-    |> Chain.join_associations(%{
-      :execution_transaction => :optional
-    })
+    # :required is used since execution records in the table are created only when
+    # the corresponding execution transaction is indexed
+    |> Chain.join_associations(%{:execution_transaction => :required})
     |> Repo.all(timeout: :infinity)
   end
 
@@ -267,7 +269,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
     query =
       from(
         lt in LifecycleTransaction,
-        where: lt.block <= ^finalized_block and lt.status == :unfinalized
+        where: lt.block_number <= ^finalized_block and lt.status == :unfinalized
       )
 
     Repo.all(query, timeout: :infinity)
@@ -291,13 +293,12 @@ defmodule Explorer.Chain.Arbitrum.Reader do
   def rollup_block_hash_to_num(block_hash) when is_binary(block_hash) do
     query =
       from(bl in BatchBlock,
-        where: bl.hash == ^block_hash
+        where: bl.block_hash == ^block_hash
       )
 
     case query
-         |> Chain.join_associations(%{
-           :block => :optional
-         })
+         # :optional here is used because situations when the block is not found are expected
+         |> Chain.join_associations(%{:block => :optional})
          |> Repo.one() do
       nil ->
         # Block with such hash is not found
@@ -357,6 +358,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       )
 
     query
+    # :required is used since the situation when commit transaction is not found is not possible
     |> Chain.join_associations(%{:commit_transaction => :required})
     |> Repo.one()
   end
@@ -374,17 +376,16 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       from(
         rb in BatchBlock,
         inner_join: fb in FullBlock,
-        on: rb.hash == fb.hash,
+        on: rb.block_hash == fb.hash,
         select: rb,
         where: not is_nil(rb.confirm_id),
         order_by: [desc: fb.number],
         limit: 1
       )
 
     case query
-         |> Chain.join_associations(%{
-           :confirm_transaction => :optional
-         })
+         # :required is used since existence of the confirmation id is checked above
+         |> Chain.join_associations(%{:confirm_transaction => :required})
          |> Repo.one() do
       nil ->
         nil
@@ -394,7 +395,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
           # `nil` and `%Ecto.Association.NotLoaded{}` indicate DB inconsistency
           nil -> nil
           %Ecto.Association.NotLoaded{} -> nil
-          confirm_transaction -> confirm_transaction.block
+          confirm_transaction -> confirm_transaction.block_number
         end
     end
   end
@@ -411,7 +412,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       from(
         rb in BatchBlock,
         inner_join: fb in FullBlock,
-        on: rb.hash == fb.hash,
+        on: rb.block_hash == fb.hash,
         select: fb.number,
         where: not is_nil(rb.confirm_id),
         order_by: [desc: fb.number],
@@ -435,8 +436,8 @@ defmodule Explorer.Chain.Arbitrum.Reader do
         tx in LifecycleTransaction,
         inner_join: ex in L1Execution,
         on: tx.id == ex.execution_id,
-        select: tx.block,
-        order_by: [desc: tx.block],
+        select: tx.block_number,
+        order_by: [desc: tx.block_number],
         limit: 1
       )
 
@@ -457,8 +458,8 @@ defmodule Explorer.Chain.Arbitrum.Reader do
         tx in LifecycleTransaction,
         inner_join: ex in L1Execution,
         on: tx.id == ex.execution_id,
-        select: tx.block,
-        order_by: [asc: tx.block],
+        select: tx.block_number,
+        order_by: [asc: tx.block_number],
         limit: 1
       )
 
@@ -483,7 +484,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       within the range, or if the block fetcher has not indexed them.
   """
   @spec unconfirmed_rollup_blocks(FullBlock.block_number(), FullBlock.block_number()) :: [
-          %{batch_number: non_neg_integer, block_num: FullBlock.block_number(), hash: Hash.t()}
+          %{batch_number: non_neg_integer, block_num: FullBlock.block_number(), block_hash: Hash.t()}
         ]
   def unconfirmed_rollup_blocks(first_block, last_block)
       when is_integer(first_block) and first_block >= 0 and
@@ -492,10 +493,10 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       from(
         rb in BatchBlock,
         inner_join: fb in FullBlock,
-        on: rb.hash == fb.hash,
+        on: rb.block_hash == fb.hash,
         select: %{
           batch_number: rb.batch_number,
-          hash: rb.hash,
+          block_hash: rb.block_hash,
           block_num: fb.number
         },
         where: fb.number >= ^first_block and fb.number <= ^last_block and is_nil(rb.confirm_id),
@@ -578,7 +579,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
       from(
         rb in BatchBlock,
         inner_join: fb in FullBlock,
-        on: rb.hash == fb.hash,
+        on: rb.block_hash == fb.hash,
         select: %{
           block_num: fb.number,
           confirm_id: rb.confirm_id
@@ -623,7 +624,7 @@ defmodule Explorer.Chain.Arbitrum.Reader do
         on: subquery.confirm_id == tx_cur.id,
         left_join: tx_prev in LifecycleTransaction,
         on: subquery.prev_confirm_id == tx_prev.id,
-        select: {tx_prev.block, tx_cur.block},
+        select: {tx_prev.block_number, tx_cur.block_number},
         where: subquery.min_block_num - 1 != subquery.prev_max_number or is_nil(subquery.prev_max_number),
         order_by: [desc: subquery.min_block_num],
         limit: 1