feat: NFT collection trigger refetch Admin API endpoint #10263
Conversation
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
113ef84 to
a159833
Compare
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
a159833 to
defadbf
Compare
vbaranov
requested review from
sl1depengwyn,
nikitosing,
varasev,
fedor-ivn and
Qwerty5Uiop
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
defadbf to
9745d66
Compare
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
9745d66 to
502fbb0
Compare
WalkthroughThe pull request introduces a comprehensive refactoring of token instance and NFT metadata handling across multiple modules in the BlockScout application. The changes primarily focus on restructuring how token instances are managed, introducing a new Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
dae7349 to
d217dba
Compare
vbaranov
requested review from
a team
and removed request for
nikitosing,
varasev,
fedor-ivn,
sl1depengwyn and
Qwerty5Uiop
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
d217dba to
fc73b26
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (11)
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (2)
6-7: Advisory onuse Indexer.Fetcher, restart: :permanent.The permanent restart strategy is appropriate for critical processes. Ensure that transient errors or one-off failures do not cause unintended restarts. A short-lived exponential backoff may help if this process faces external dependencies that can fail intermittently.
38-47: Check error handling inrun/2.Currently, if
batch_fetch_instances/1encounters a failure, it doesn't appear to return an error or halt. Confirm that this is intentional. If partial failures need to be captured or retried, consider implementing robust error handling or logging.apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex (2)
8-8: Duplicate alias forExplorer.Chain.Token.Instance.Line 8 aliases
Instance, and the subsequent line also aliases it as part ofExplorer.Chain.{..., Token.Instance}. This duplicate alias might override or cause confusion. Consider removing one for clarity.- alias Explorer.Chain.Token.Instance alias Explorer.Chain.{Address, BridgedToken, Token, Token.Instance}
369-388: Security note ontrigger_nft_collection_metadata_refetch/2.It strictly compares
params["api_key"]to the configured:sensitive_endpoints_api_key. This is fine for basic protection, but consider hashing or rotating the key for stronger security. If the key is absent or mismatched, the function short-circuits. Confirm that logs do not accidentally expose the key.apps/explorer/lib/explorer/chain/token/instance.ex (1)
891-902:mark_nft_collection_to_refetch/1setsmetadatato nil anderrorto the sentinel.This is a simple bulk approach, but be mindful that large-scale updates could be expensive on large tables. For bigger data sets, consider chunking or pagination to avoid lock contention.
apps/indexer/lib/indexer/fetcher/on_demand/token_metadata_refetch.ex (2)
19-21:fetch_metadata/2callsmark_nft_collection_to_refetch/1.This is a minimal approach that unconditionally marks the entire NFT collection for refetch. If partial refetch or selective logic is needed, consider more granular design.
32-37:handle_castcallback in a permanent fetcher process.The process does not reply to the caller and simply updates the DB state. That's fine for a basic "fire-and-forget" design. Consider adding logs or metrics to track successful triggers or errors.
docker-compose/envs/common-blockscout.env (2)
176-176: Consider environment-specific toggles for refetch fetcher.Enabling the
INDEXER_DISABLE_TOKEN_INSTANCE_REFETCH_FETCHERenvironment variable by default may trigger unintentional re-fetch operations for local development. You might consider toggling it per environment to efficiently control resource usage.
201-202: Uncomment or remove these lines based on usage needs.These lines hint at possible concurrency/batch configurations. If these features are indeed required, consider uncommenting and assigning meaningful values; otherwise, removing them prevents confusion.
apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs (1)
1668-1733: Tests for triggering NFT collection metadata refetch are comprehensive.
- Success case: Validates the proper clearance of existing token instance metadata, followed by a 200 response.
- Unauthorized case: Verifies 401 when no API key is provided.
These tests thoroughly cover positive and negative scenarios. Additionally, consider asserting that no operations occur if an invalid API key is provided (malformed vs. missing).
config/runtime.exs (1)
897-900: Validate concurrency and batch size configuration.The
INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCYandINDEXER_TOKEN_INSTANCE_REFETCH_BATCH_SIZEsettings give valuable tuning options. Ensure these environment variables are well-documented and comprehensively tested with varying loads to prevent potential bottlenecks or excessive memory usage.
🛑 Comments failed to post (2)
apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex (1)
44-44: 💡 Codebase verification
Parameter order is incorrect in the function call
The function call
Instance.token_instance_with_unfetched_metadata?(token_id, hash)has incorrect parameter order. Based on the function definition inapps/explorer/lib/explorer/chain/token/instance.ex, the function expects:
- First parameter:
token_id(✓ correct)- Second parameter:
token_contract_address(✗ incorrect variable name used)- Optional third parameter:
options(defaulting to[])The variable
hashshould be renamed totoken_contract_addressfor clarity and consistency with the function's signature.🔗 Analysis chain
Check parameter order for correctness.
Although referencing the new alias ensures clarity, confirm that the arguments
token_id, hashinInstance.token_instance_with_unfetched_metadata?match its expected parameter order.Run the following script to locate the function definition and confirm the parameter order:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if token_instance_with_unfetched_metadata? expects token_id, then contract_address_hash. ast-grep --pattern $'def token_instance_with_unfetched_metadata?(token_id, contract_address_hash) { $$$ }'Length of output: 107
Script:
#!/bin/bash # Let's try to find the function definition using ripgrep rg -p "token_instance_with_unfetched_metadata\?" --type elixirLength of output: 614
Script:
#!/bin/bash # Let's see the implementation and context of the function rg -A 10 "token_instance_with_unfetched_metadata\?" apps/explorer/lib/explorer/chain/token/instance.exLength of output: 603
apps/indexer/lib/indexer/fetcher/token_instance/retry.ex (1)
34-34: 🛠️ Refactor suggestion
Potential backlog or queue size considerations.
While streaming token instances with errors, the current trampoline call with
:timer.sleep/1might lead to performance overhead if the queue remains full for extended periods. Consider a backpressure or pushback mechanism to avoid potential busy-wait scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
docker-compose/envs/common-blockscout.env (1)
202-202: Consider adding a default value for INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY.Other concurrency settings in the file typically have default values. Adding a default value would help maintain consistency and provide guidance for deployment.
-# INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY= +# INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY=10config/runtime.exs (1)
897-900: LGTM! Consider documenting the environment variables.The configuration for TokenInstance.Refetch is well-structured with appropriate defaults for concurrency and batch size. The implementation follows the established patterns in the codebase.
Consider adding these environment variables to the documentation:
- INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY (default: 10)
- INDEXER_TOKEN_INSTANCE_REFETCH_BATCH_SIZE (default: 10)
apps/explorer/test/explorer/chain/token/instance_test.exs (1)
155-208: Consider adding more pagination test casesWhile the current test effectively verifies basic pagination functionality, consider adding tests for:
- Empty result set
- Last page scenario
- Maximum page size handling
- Invalid pagination key handling
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (3)
1-4: Documentation could be more descriptiveThe @moduledoc could be enhanced to better describe:
- When/why a token instance is marked for re-fetching
- The relationship with BufferedTask behavior
- The expected outcomes of the re-fetch process
Consider expanding the documentation like this:
@moduledoc """ - Fetches information about a token instance, which is marked to be re-fetched. + Fetches information about token instances that are marked for metadata re-fetching. + + This module implements the `BufferedTask` behavior to efficiently process multiple + token instances in batches. It monitors instances flagged for re-fetching and + updates their metadata through the batch_fetch_instances process. + + Re-fetching is typically triggered when: + - Token metadata is found to be incomplete + - Previous fetch attempts failed + - Manual refresh is requested via the Admin API """
38-47: Add logging for better observabilityThe
run/2function should log the number of instances being processed and any failures during batch_fetch_instances for better debugging.@impl BufferedTask def run(token_instances, _) when is_list(token_instances) do + Logger.debug("Processing #{length(token_instances)} token instances for refetch") + token_instances |> Enum.filter(fn %{contract_address_hash: hash, token_id: token_id} -> Instance.token_instance_marked_to_refetch?(token_id, hash) end) - |> batch_fetch_instances() + |> case do + [] -> + Logger.debug("No instances marked for refetch") + :ok + instances_to_fetch -> + Logger.info("Fetching metadata for #{length(instances_to_fetch)} instances") + batch_fetch_instances(instances_to_fetch) + end :ok end
49-57: Consider making configuration more flexibleThe defaults function could benefit from more configuration options being externalized to the application config.
defp defaults do [ - flush_interval: :infinity, + flush_interval: Application.get_env(:indexer, __MODULE__)[:flush_interval] || :infinity, max_concurrency: Application.get_env(:indexer, __MODULE__)[:concurrency] || @default_max_concurrency, max_batch_size: Application.get_env(:indexer, __MODULE__)[:batch_size] || @default_max_batch_size, - poll: false, + poll: Application.get_env(:indexer, __MODULE__)[:poll] || false, task_supervisor: __MODULE__.TaskSupervisor ] endapps/explorer/lib/explorer/chain/token/instance.ex (1)
Line range hint
96-128: Query functions are well-structured but could benefit from more documentation.The query functions are logically organized, but some functions like
page_token_instance/2lack documentation.Consider adding
@docstrings to these functions following the module's documentation format:@doc """ Pages token instances based on the provided paging options. ## Parameters - `query`: The base query to page - `paging_options`: The PagingOptions struct containing paging parameters ## Returns - A modified query with paging conditions applied """ def page_token_instance(query, %PagingOptions{key: {token_id}, asc_order: true}) do where(query, [i], i.token_id > ^token_id) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex(5 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex(1 hunks)apps/explorer/lib/explorer/chain.ex(0 hunks)apps/explorer/lib/explorer/chain/token/instance.ex(16 hunks)apps/explorer/test/explorer/chain/token/instance_test.exs(2 hunks)apps/explorer/test/explorer/chain_test.exs(0 hunks)apps/indexer/lib/indexer/application.ex(3 hunks)apps/indexer/lib/indexer/fetcher/on_demand/token_metadata_refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/helper.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/retry.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex(3 hunks)apps/indexer/lib/indexer/supervisor.ex(2 hunks)config/runtime.exs(2 hunks)docker-compose/envs/common-blockscout.env(2 hunks)
💤 Files with no reviewable changes (2)
- apps/explorer/test/explorer/chain_test.exs
- apps/explorer/lib/explorer/chain.ex
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex
- apps/indexer/lib/indexer/fetcher/token_instance/retry.ex
- apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex
- apps/indexer/lib/indexer/fetcher/token_instance/helper.ex
- apps/indexer/lib/indexer/supervisor.ex
- apps/indexer/lib/indexer/fetcher/on_demand/token_metadata_refetch.ex
- apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex
🧰 Additional context used
📓 Path-based instructions (4)
apps/indexer/lib/indexer/application.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/explorer/lib/explorer/chain/token/instance.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
🔇 Additional comments (12)
docker-compose/envs/common-blockscout.env (1)
176-176: LGTM! New configuration variables for token instance refetching.The new environment variables align well with the PR objectives and follow consistent naming patterns with other similar configurations in the file.
Also applies to: 201-202
config/runtime.exs (1)
847-849: LGTM! Supervisor configuration follows established patterns.The configuration for TokenInstance.Refetch.Supervisor follows the same pattern as other supervisor configurations in the codebase, using the standard environment variable parsing helper.
apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex (1)
Line range hint
219-234: LGTM! Clean refactoring of NFT instance retrieval.The change from
ChaintoInstancemodule is well-implemented with proper error handling and maintains the expected behavior.Let's verify that all usages of the old
Chain.nft_instance_from_token_id_and_token_addresshave been updated:✅ Verification successful
All references to NFT instance retrieval have been updated to use the new Instance module
The verification shows that:
- No references to the old
Chain.nft_instance_from_token_id_and_token_addressexist in the codebase- The new
Instance.nft_instance_by_token_id_and_token_addressis consistently used across all relevant files:
- Main implementation in
explorer/lib/explorer/chain/token/instance.ex- Test coverage in
explorer/test/explorer/chain/token/instance_test.exs- Usage in API controllers and views:
block_scout_web/views/api/v2/address_view.exblock_scout_web/controllers/tokens/instance/*_controller.exblock_scout_web/controllers/api/v2/token_controller.ex🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old Chain module function # Test: Search for old function name. Expect: No results. rg "nft_instance_from_token_id_and_token_address" # Test: Verify consistent usage of new Instance module function # Expect: All NFT instance retrievals use the new function rg "nft_instance_by_token_id_and_token_address"Length of output: 1610
apps/explorer/test/explorer/chain/token/instance_test.exs (2)
6-6: LGTM: PagingOptions alias added for pagination testsThe addition of the PagingOptions alias is appropriate as it's used in the new pagination tests.
85-153: Well-structured test coverage for upsert_token_instance/1The test cases comprehensively cover:
- Success path with valid parameters
- Validation of required fields
- Error state handling
- Error state transitions
apps/indexer/lib/indexer/application.ex (2)
36-39: LGTM! Pool size calculation properly integratedThe pool size calculation for the new Refetch fetcher is correctly integrated with the existing calculations.
51-51: Verify supervisor initialization orderThe TokenMetadataRefetch supervisor is added to base_children. Ensure there are no dependencies that would require a specific initialization order.
✅ Verification successful
Let me analyze the dependencies further by checking the supervisor implementation and any potential dependencies in the token metadata refetch module.
Let me check the supervisor implementation since we haven't found it in the current file.
Let me check one final thing to see if there are any dependencies in the chain of calls.
Supervisor initialization order is safe
The TokenMetadataRefetch service is a simple GenServer that only depends on the database layer through
Explorer.Chain.Token.Instance. It doesn't have any dependencies on other supervisors or services that would require a specific initialization order. The service:
- Uses GenServer for its implementation
- Only performs database operations through Ecto
- Operates independently of other services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential supervisor dependencies rg -A 5 "TokenMetadataRefetch" --type elixirLength of output: 4388
Script:
#!/bin/bash # Check supervisor implementation and dependencies ast-grep --pattern 'defmodule $_ do $$$ use Supervisor $$$ end' apps/indexer/lib/indexer/fetcher/on_demand/token_metadata_refetch.ex # Check for any dependencies on other supervisors or services cat apps/indexer/lib/indexer/fetcher/on_demand/token_metadata_refetch.exLength of output: 1113
Script:
#!/bin/bash # Look for the supervisor implementation fd "token_metadata_refetch" --type f --exec cat {}Length of output: 950
Script:
#!/bin/bash # Check for dependencies in the chain of calls rg "mark_nft_collection_to_refetch" --type elixir -A 5Length of output: 1350
apps/explorer/lib/explorer/chain/token/instance.ex (5)
15-22: Well-structured module attributes and types!The new constants and types are well-defined:
- Default page size and paging options are set to reasonable values
- The
api?type is properly defined for API-related function options- The
@marked_to_refetchconstant is appropriately defined for the metadata refetch feature
Line range hint
67-85: Good use of__MODULE__for better maintainability!The changeset function has been updated to use
__MODULE__instead of hardcoding the module name, which improves maintainability.
1072-1083: Well-implemented token instance fetching with proper preloading!The
address_to_unique_tokens/3function is well-structured with:
- Proper use of paging options with defaults
- Efficient preloading of associations
- Clear mapping of results with owner information
881-988: Well-implemented metadata refetch functionality with proper error handling!The new functions for metadata refetch are well-structured with:
- Clear documentation following the module's format
- Proper error handling and status tracking
- Efficient database queries
1157-1220: Sophisticated retry mechanism with potential edge case.The retry logic is well-implemented with exponential backoff, but there's a potential edge case to consider.
The retry mechanism uses exponential backoff with a maximum retry count, but we should verify the behavior when approaching the maximum value of a smallint (32767).
Additionally, consider adding a comment explaining the exponential backoff formula:
# The retry interval is calculated as: coef * base^min(retries_count + 1, max_retry_count) # where max_retry_count is derived from max_refetch_interval to prevent overflow✅ Verification successful
Let me gather more information about the retry mechanism implementation.
Let me check the migration files to understand how retries_count is defined in the database schema.
The retry mechanism is safe from integer overflow
The concern about hitting the smallint limit is already addressed in the codebase:
- The
retries_countis defined as a:smallintin the database schema, which has a range of -32768 to 32767- The code defines
@max_retries_count_value 32767and uses it with theerror_to_ban_intervalmapping- The token instance becomes banned when
retries_count + 1 > max_retries_count_before_ban, which prevents further retries- The exponential backoff formula is bounded by
max_retry_countwhich is derived frommax_refetch_intervalThese safeguards ensure that:
- The retries will stop well before reaching the smallint limit
- The instance will be marked as banned, preventing further increment of
retries_count- The exponential backoff is capped by both the retry count and maximum interval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any instances close to max retries # that might hit the smallint limit rg -A 5 "retries_count.*32767" .Length of output: 496
Script:
#!/bin/bash # Check the schema definition for retries_count ast-grep --pattern 'field :retries_count, $_' # Check if there are any constraints or validations on retries_count rg -B2 -A2 'retries_count.*validate' . # Look for migrations related to retries_count fd -e exs -x grep -l 'retries_count' {}Length of output: 674
Script:
#!/bin/bash # Check the migration files content cat ./apps/explorer/priv/repo/migrations/20240503091708_add_nft_instance_fetcher_aux_fields.exs cat ./apps/explorer/priv/repo/migrations/20240828140638_add_token_balance_retry_fields.exsLength of output: 720
| describe "nft_instance_by_token_id_and_token_address/2" do | ||
| test "return NFT instance" do | ||
| token = insert(:token) | ||
|
|
||
| token_id = 10 | ||
|
|
||
| insert(:token_instance, | ||
| token_contract_address_hash: token.contract_address_hash, | ||
| token_id: token_id | ||
| ) | ||
|
|
||
| assert {:ok, result} = | ||
| Chain.nft_instance_by_token_id_and_token_address( | ||
| token_id, | ||
| token.contract_address_hash | ||
| ) | ||
|
|
||
| assert result.token_id == Decimal.new(token_id) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case tests for NFT instance retrieval
The current test only covers the successful retrieval case. Consider adding tests for:
- Non-existent token_id
- Invalid token contract address
- Invalid token_id format (non-numeric)
Example structure for additional tests:
test "returns error for non-existent token_id" do
token = insert(:token)
token_id = 999
assert {:error, :not_found} =
Chain.nft_instance_by_token_id_and_token_address(
token_id,
token.contract_address_hash
)
end| @impl BufferedTask | ||
| def init(initial_acc, reducer, _) do | ||
| {:ok, acc} = | ||
| Instance.stream_token_instances_marked_to_refetch(initial_acc, fn data, acc -> | ||
| reducer.(data, acc) | ||
| end) | ||
|
|
||
| acc | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for stream_token_instances_marked_to_refetch
The init/3 function assumes the stream operation will always succeed. Consider handling potential database errors.
@impl BufferedTask
def init(initial_acc, reducer, _) do
- {:ok, acc} =
- Instance.stream_token_instances_marked_to_refetch(initial_acc, fn data, acc ->
- reducer.(data, acc)
- end)
+ case Instance.stream_token_instances_marked_to_refetch(initial_acc, fn data, acc ->
+ reducer.(data, acc)
+ end) do
+ {:ok, acc} -> acc
+ {:error, reason} ->
+ Logger.error("Failed to stream token instances: #{inspect(reason)}")
+ initial_acc
+ end
- acc
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @impl BufferedTask | |
| def init(initial_acc, reducer, _) do | |
| {:ok, acc} = | |
| Instance.stream_token_instances_marked_to_refetch(initial_acc, fn data, acc -> | |
| reducer.(data, acc) | |
| end) | |
| acc | |
| end | |
| @impl BufferedTask | |
| def init(initial_acc, reducer, _) do | |
| case Instance.stream_token_instances_marked_to_refetch(initial_acc, fn data, acc -> | |
| reducer.(data, acc) | |
| end) do | |
| {:ok, acc} -> acc | |
| {:error, reason} -> | |
| Logger.error("Failed to stream token instances: #{inspect(reason)}") | |
| initial_acc | |
| end | |
| end |
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
2 times, most recently
from
e08526d to
3f4f022
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex (1)
369-388: Provide documentation for the new public endpoint & consider header-based API key.
- Add a
@docto clarify the purpose and usage of this new endpoint.- Consider placing the sensitive API key in a header instead of the request body for better security and potential logging avoidance.
apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex (1)
14-17: Add @doc for public function.
trigger_refetch/1is a public API for this GenServer, so consider adding a short@doccomment to clarify its purpose and usage.@doc """ Triggers a refetch operation for the given token. ## Parameters - `token`: The token struct whose metadata needs to be refetched. ## Returns - `:ok` """ @spec trigger_refetch(Token.t()) :: :ok def trigger_refetch(token) do ... enddocker-compose/envs/common-blockscout.env (1)
201-202: Clarify or remove commented environment variables.If these vars are desired, provide defaults or uncomment them; otherwise, remove to avoid confusion.
-# INDEXER_TOKEN_INSTANCE_REFETCH_BATCH_SIZE=10 -# INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY= +INDEXER_TOKEN_INSTANCE_REFETCH_BATCH_SIZE=10 +INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY=5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex(5 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex(1 hunks)apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs(2 hunks)apps/explorer/lib/explorer/chain.ex(0 hunks)apps/explorer/lib/explorer/chain/token/instance.ex(16 hunks)apps/explorer/test/explorer/chain/token/instance_test.exs(2 hunks)apps/explorer/test/explorer/chain_test.exs(0 hunks)apps/indexer/lib/indexer/application.ex(3 hunks)apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/helper.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/retry.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex(3 hunks)apps/indexer/lib/indexer/supervisor.ex(2 hunks)config/runtime.exs(2 hunks)docker-compose/envs/common-blockscout.env(2 hunks)
💤 Files with no reviewable changes (2)
- apps/explorer/lib/explorer/chain.ex
- apps/explorer/test/explorer/chain_test.exs
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex
- apps/indexer/lib/indexer/application.ex
- apps/indexer/lib/indexer/supervisor.ex
- apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex
- apps/indexer/lib/indexer/fetcher/token_instance/helper.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex
- apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex
- apps/indexer/lib/indexer/fetcher/token_instance/retry.ex
- apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex
🧰 Additional context used
📓 Path-based instructions (4)
apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/explorer/lib/explorer/chain/token/instance.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
🔇 Additional comments (17)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex (5)
11-11: Alias addition looks good.No concerns here; referencing
NFTCollectionMetadataRefetchOnDemandaligns with the new functionality.
201-202: Implementation looks correct.This call nicely leverages the new
nft_instance_by_token_id_and_token_addressfunction, and thewithstatement handles errors gracefully.
206-206: Owner assignment logic looks good.Attaching the token owner within the pipeline is a clean approach.
355-356: Refetch metadata call integrated properly.This code reuses
nft_instance_by_token_id_and_token_addressunder the existingwitherror flow, which is aligned with the refactor.
178-178: Verify usage consistency.Switching to
Instance.address_to_unique_tokensis consistent with the refactor. Verify that any other references toChain.address_to_unique_tokenshave also been appropriately updated.✅ Verification successful
All references to
address_to_unique_tokenshave been properly updatedThe verification shows that all instances of
address_to_unique_tokensare now using theInstancemodule instead ofChain. The function appears in three locations:
apps/explorer/test/explorer/chain/token/instance_test.exsapps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.exapps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.exAll of these are using the new
Instance.address_to_unique_tokensimplementation, and there are no remaining references toChain.address_to_unique_tokensin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching references to the old function usage to confirm they have all been replaced rg "Chain\.address_to_unique_tokens"Length of output: 38
Script:
#!/bin/bash # Let's search for both old and new function references to get a complete picture echo "Searching for Instance.address_to_unique_tokens:" rg "Instance\.address_to_unique_tokens" -A 2 echo -e "\nSearching for any remaining Chain references in token controller:" fd "token_controller.ex" --exec rg "Chain\." {} -A 2Length of output: 10449
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (1)
28-36: Same comment about error handling for DB streaming.apps/explorer/test/explorer/chain/token/instance_test.exs (4)
6-6: Alias addition is fine.No objection to bringing
Explorer.PagingOptionsinto scope here.
85-153: Upsert token instance tests look thorough.Covers valid and invalid scenarios, including inserting errors and clearing them.
155-208: Pagination test confirmed.Good approach for verifying scenario-based pagination.
210-229: Add negative scenario tests for NFT instance lookups.docker-compose/envs/common-blockscout.env (1)
176-176: Enabling token instance refetch fetcher is fine.Make sure it’s documented in deployment notes.
apps/explorer/lib/explorer/chain/token/instance.ex (4)
15-22: LGTM! Well-structured constant and type declarations.The new constants and types are well-defined with appropriate values for pagination and metadata refetching.
881-902: LGTM! Well-implemented metadata refetch function.The
mark_nft_collection_to_refetch/1function is well-documented and efficiently implemented using batch updates with proper error handling.
904-938: LGTM! Well-designed streaming functions.The streaming functions are efficiently implemented using
stream_reducewith proper documentation and error handling.
940-988: LGTM! Well-implemented helper functions.The helper functions are efficiently implemented using
exists?queries with proper documentation and type specifications.apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs (1)
1668-1733: LGTM! Comprehensive test coverage for the new endpoint.The tests thoroughly verify:
- Successful metadata refetch with API key
- Unauthorized access handling
- Database state verification
config/runtime.exs (1)
Line range hint
847-900: LGTM! Well-structured configuration for the new fetcher.The configuration properly defines:
- Supervisor settings with disable flag
- Fetcher settings with configurable concurrency and batch size
| @@ -5,8 +5,10 @@ defmodule BlockScoutWeb.API.V2.TokenController do | |||
| alias BlockScoutWeb.{AccessHelper, CaptchaHelper} | |||
| alias BlockScoutWeb.API.V2.{AddressView, TransactionView} | |||
| alias Explorer.{Chain, Helper, PagingOptions} | |||
| alias Explorer.Chain.Token.Instance | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicated alias.
Lines 8 and 9 both alias Explorer.Chain.Token.Instance, which is redundant. Consider removing one to avoid confusion.
- alias Explorer.Chain.Token.Instance
alias Explorer.Chain.{Address, BridgedToken, Token, Token.Instance}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alias Explorer.Chain.Token.Instance | |
| alias Explorer.Chain.{Address, BridgedToken, Token, Token.Instance} |
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
3f4f022 to
26cf22c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
config/runtime.exs (1)
897-900: LGTM! Consider documenting the environment variables.The configuration for the token instance refetch fetcher is well-structured with reasonable defaults for concurrency and batch size. However, it would be beneficial to document these environment variables in the project's documentation to help with deployment configuration.
Environment variables to document:
- INDEXER_TOKEN_INSTANCE_REFETCH_CONCURRENCY
- INDEXER_TOKEN_INSTANCE_REFETCH_BATCH_SIZE
apps/explorer/test/explorer/chain/token/instance_test.exs (3)
85-153: Add more test cases for comprehensive coverage.While the current test cases cover basic scenarios, consider adding these additional test cases for better coverage:
- Updating an existing instance with different metadata
- Handling of invalid token_id formats (non-numeric values)
- Handling of non-existent token contract addresses
Example structure for additional test:
test "updates existing instance with new metadata" do token = insert(:token) # Insert initial instance {:ok, instance} = Instance.upsert_token_instance(%{ token_id: 1, token_contract_address_hash: token.contract_address_hash, metadata: %{uri: "http://example.com"} }) # Update with new metadata {:ok, updated} = Instance.upsert_token_instance(%{ token_id: 1, token_contract_address_hash: token.contract_address_hash, metadata: %{uri: "http://new-example.com"} }) assert updated.id == instance.id assert updated.metadata.uri == "http://new-example.com" end
155-208: Enhance pagination test coverage.The current test verifies basic pagination functionality, but consider adding these test cases:
- Empty result set scenario
- Invalid pagination parameters handling
- Maximum page size limits
Example structure for empty result test:
test "returns empty list when no tokens exist" do token_contract_address = insert(:contract_address) token = insert(:token, contract_address: token_contract_address, type: "ERC-721") paging_options = %PagingOptions{page_size: 10} result = token_contract_address.hash |> Instance.address_to_unique_tokens(token, paging_options: paging_options) |> Enum.to_list() assert result == [] end
Line range hint
1-229: Improve test file organization and maintainability.Consider these structural improvements:
- Use
setupblocks for common test data (token and contract address creation)- Add documentation for complex test scenarios
- Maintain consistent assertion style throughout tests
Example setup block:
describe "token instance operations" do setup do token_contract_address = insert(:contract_address) token = insert(:token, contract_address: token_contract_address, type: "ERC-721") %{ token: token, contract_address: token_contract_address } end test "test case", %{token: token} do # Test implementation end endapps/indexer/test/indexer/fetcher/token_instance/refetch_test.exs (4)
1-3: Consider adding a module-level docstring.
Providing a short@moduledoccan help clarify the test module’s purpose and usage, making it easier to navigate.
8-20: Expand edge case testing forchild_spec/1.
Currently, the test verifies merging default options with a provided list. Consider adding a scenario that tests behavior with empty or missing options or conflicting keys to ensure robust coverage.
22-56: Good use ofinit/3for initialization logic.
The test verifies that only token instances marked with a specific error state get refetched. It might be beneficial to include a case where the error value is something other than:marked_to_refetch, to confirm that such tokens are excluded from this flow.
58-82: Add negative or no-token tests forrun/2.
Testing how the function behaves when no tokens are provided or when tokens do not match the:marked_to_refetchcondition would strengthen confidence in the refetch process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex(5 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex(1 hunks)apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs(2 hunks)apps/explorer/lib/explorer/chain.ex(0 hunks)apps/explorer/lib/explorer/chain/token/instance.ex(16 hunks)apps/explorer/test/explorer/chain/token/instance_test.exs(2 hunks)apps/explorer/test/explorer/chain_test.exs(0 hunks)apps/indexer/lib/indexer/application.ex(3 hunks)apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/helper.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/retry.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex(3 hunks)apps/indexer/lib/indexer/supervisor.ex(2 hunks)apps/indexer/test/indexer/fetcher/token_instance/refetch_test.exs(1 hunks)config/runtime.exs(2 hunks)docker-compose/envs/common-blockscout.env(2 hunks)
💤 Files with no reviewable changes (2)
- apps/explorer/test/explorer/chain_test.exs
- apps/explorer/lib/explorer/chain.ex
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex
- apps/indexer/lib/indexer/fetcher/token_instance/retry.ex
- apps/indexer/lib/indexer/fetcher/token_instance/helper.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex
- docker-compose/envs/common-blockscout.env
- apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex
- apps/indexer/lib/indexer/application.ex
- apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex
- apps/indexer/lib/indexer/supervisor.ex
- apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex
- apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex
- apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex
👮 Files not reviewed due to content moderation or server errors (3)
- apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex
- apps/explorer/lib/explorer/chain/token/instance.ex
- apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs
🧰 Additional context used
📓 Path-based instructions (2)
apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
apps/explorer/lib/explorer/chain/token/instance.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
🔇 Additional comments (3)
config/runtime.exs (1)
847-849: LGTM! Supervisor configuration looks good.The configuration for the token instance refetch supervisor follows the established pattern and integrates well with the existing configuration structure.
apps/explorer/test/explorer/chain/token/instance_test.exs (1)
210-229: Add error case tests for NFT instance retrieval.The current test only covers the successful retrieval case. Consider adding tests for:
- Non-existent token_id
- Invalid token contract address
- Invalid token_id format (non-numeric)
Example structure for additional tests:
test "returns error for non-existent token_id" do token = insert(:token) token_id = 999 assert {:error, :not_found} = Instance.nft_instance_by_token_id_and_token_address( token_id, token.contract_address_hash ) endapps/indexer/test/indexer/fetcher/token_instance/refetch_test.exs (1)
4-7: Aliases are well-structured.
These aliases make the code more readable and highlight the relevant modules clearly. No further changes necessary here.
vbaranov
force-pushed
the
vb-nft-collection-trigger-metadata-refetch-admin-api-endpoint
branch
from
26cf22c to
ea06a22
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/explorer/test/explorer/chain/token/instance_test.exs (1)
210-229: 🛠️ Refactor suggestionAdd comprehensive test cases for NFT instance retrieval
The current test only covers the successful case. As mentioned in the previous review, add tests for error cases. Additionally, consider:
- Validation of all instance fields, not just token_id
- Case sensitivity in contract address
- Performance with large token_ids
Example structure (in addition to previous suggestions):
test "validates case sensitivity in contract address" do token = insert(:token) token_id = 10 insert(:token_instance, token_contract_address_hash: token.contract_address_hash, token_id: token_id ) uppercase_address = String.upcase(to_string(token.contract_address_hash)) assert {:ok, result} = Instance.nft_instance_by_token_id_and_token_address( token_id, uppercase_address ) assert result.token_id == Decimal.new(token_id) end
🧹 Nitpick comments (8)
apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs (2)
1693-1725: Consider adding edge case tests.While the happy path test is thorough, consider adding tests for:
- Token with no instances
- Token with a large number of instances (pagination test)
- Different token types (ERC-1155, ERC-404)
1727-1734: Consider additional error cases.The unauthorized test is good, but consider adding tests for:
- Invalid token address
- Non-existent token
- Invalid API key format
apps/explorer/test/explorer/chain/token/instance_test.exs (3)
85-153: Enhance test coverage forupsert_token_instance/1Consider adding these test cases to improve coverage:
- Edge cases for token_id:
- Maximum/minimum allowed values
- Non-numeric values
- Metadata validation:
- Invalid URI format
- Missing required metadata fields
- Concurrent upsert operations:
- Race condition handling
Example structure:
test "handles maximum token_id value" do token = insert(:token) max_token_id = # Define your max value params = %{ token_id: max_token_id, token_contract_address_hash: token.contract_address_hash, metadata: %{uri: "http://example.com"} } {:ok, result} = Instance.upsert_token_instance(params) assert result.token_id == Decimal.new(max_token_id) end
155-208: Enhance pagination test coverageThe current test only covers basic forward pagination. Consider adding:
- Backward pagination test
- Empty result set test
- Verification of complete token instance data, not just token_id
- Edge cases for page_size
Example structure:
test "handles empty result set with pagination" do token_contract_address = insert(:contract_address) token = insert(:token, contract_address: token_contract_address, type: "ERC-721") paging_options = %PagingOptions{page_size: 10} result = token_contract_address.hash |> Instance.address_to_unique_tokens(token, paging_options: paging_options) |> Enum.to_list() assert result == [] end
Line range hint
1-6: Add module documentation for test strategyConsider adding module documentation to:
- Describe the test strategy and coverage goals
- List any assumptions or limitations
- Explain the test data setup approach
Example:
defmodule Explorer.Chain.Token.InstanceTest do @moduledoc """ Test suite for Token Instance operations. ## Test Strategy - Covers CRUD operations for token instances - Validates pagination and retrieval mechanisms - Tests error conditions and edge cases ## Test Data Setup Fixtures are used to create tokens and instances with specific attributes needed for each test scenario. """ use Explorer.DataCase # ... rest of the codeapps/explorer/lib/explorer/chain/token/instance.ex (3)
922-924: Fix copy-pasted documentation comment.The documentation for
stream_token_instances_marked_to_refetch/2appears to be copied fromstream_token_instances_with_unfetched_metadata/2. It should be updated to accurately describe its specific purpose of streaming instances marked for refetch.
1132-1195: Consider extracting complex SQL logic into named functions.The token instance metadata update logic uses complex SQL fragments that could benefit from being extracted into named functions for better maintainability and testability. Consider creating helper functions for:
- Retry count calculation
- Ban status determination
- Refetch interval computation
993-1022: Consider externalizing error priority configuration.The error priority lists are hardcoded in the function. Consider moving them to the configuration:
+ @high_priority_errors ["request error: 429", ":checkout_timeout"] + @negative_priority_errors ["VM execution error", "no uri", "invalid json"] def stream_token_instances_with_error(initial, reducer, limited? \\ false) when is_function(reducer, 2) do - # likely to get valid metadata - high_priority = ["request error: 429", ":checkout_timeout"] - # almost impossible to get valid metadata - negative_priority = ["VM execution error", "no uri", "invalid json"] __MODULE__ |> where([instance], is_nil(instance.is_banned) or not instance.is_banned) |> where([instance], not is_nil(instance.error)) |> where([instance], instance.error != ^@marked_to_refetch) |> where([instance], is_nil(instance.refetch_after) or instance.refetch_after < ^DateTime.utc_now()) |> select([instance], %{ contract_address_hash: instance.token_contract_address_hash, token_id: instance.token_id }) |> order_by([instance], asc: instance.refetch_after, - desc: instance.error in ^high_priority, - asc: instance.error in ^negative_priority + desc: instance.error in ^@high_priority_errors, + asc: instance.error in ^@negative_priority_errors )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex(5 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex(2 hunks)apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex(1 hunks)apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs(2 hunks)apps/explorer/lib/explorer/chain.ex(0 hunks)apps/explorer/lib/explorer/chain/token/instance.ex(16 hunks)apps/explorer/test/explorer/chain/token/instance_test.exs(2 hunks)apps/explorer/test/explorer/chain_test.exs(0 hunks)apps/indexer/lib/indexer/application.ex(3 hunks)apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/helper.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex(1 hunks)apps/indexer/lib/indexer/fetcher/token_instance/retry.ex(2 hunks)apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex(3 hunks)apps/indexer/lib/indexer/supervisor.ex(2 hunks)apps/indexer/test/indexer/fetcher/token_instance/refetch_test.exs(1 hunks)config/runtime.exs(2 hunks)docker-compose/envs/common-blockscout.env(2 hunks)
💤 Files with no reviewable changes (2)
- apps/explorer/test/explorer/chain_test.exs
- apps/explorer/lib/explorer/chain.ex
🚧 Files skipped from review as they are similar to previous changes (16)
- apps/indexer/lib/indexer/fetcher/token_instance/retry.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex
- apps/indexer/lib/indexer/fetcher/on_demand/nft_collection_metadata_refetch.ex
- apps/indexer/lib/indexer/fetcher/token_instance/realtime.ex
- apps/indexer/lib/indexer/fetcher/token_instance/helper.ex
- docker-compose/envs/common-blockscout.env
- apps/indexer/lib/indexer/application.ex
- apps/indexer/lib/indexer/supervisor.ex
- apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex
- apps/indexer/lib/indexer/fetcher/token_instance/refetch.ex
- apps/block_scout_web/lib/block_scout_web/views/api/v2/address_view.ex
- apps/indexer/lib/indexer/fetcher/token_instance/sanitize.ex
- apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex
- apps/indexer/test/indexer/fetcher/token_instance/refetch_test.exs
🧰 Additional context used
📓 Path-based instructions (1)
apps/explorer/lib/explorer/chain/token/instance.ex (1)
Pattern **/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """
This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars.
The next lines in the documentation comment of any function must be wrapped
after 80 chars (soft limit). It will allow to read documentation comments
well in the code editors.
## Parameters
- `param1`: Description of param1
- `param2`: Description of param2
## Returns
- what the function returns in one case
- what the function returns if there is more than one possible outcome
"""- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """
Provides an example function.
## Parameters
- `integer`: An integer input.
## Returns
- A string result.
"""
@spec example_function(integer()) :: String.t()
def example_function(1), do: "One"
def example_function(2), do: "Two"- Behavior-Implementing Function:
# Handles an example call.
@impl GenServer
def handle_call(:example, _from, state) do
{:reply, :ok, state}
end- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok- Do not raise a comment for a missing @SPEC.
🔇 Additional comments (10)
config/runtime.exs (2)
847-849: LGTM! Configuration for token instance refetch supervisorThe configuration aligns with the PR objectives for NFT collection metadata refetch functionality, following the established pattern for supervisor configurations.
897-900: LGTM! Configuration for token instance refetch fetcherThe configuration properly sets up concurrency and batch size parameters for the NFT collection metadata refetch feature, with reasonable defaults that can be overridden via environment variables.
apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs (2)
15-15: LGTM! Alias follows consistent pattern.The alias for
NFTCollectionMetadataRefetchis properly placed and follows the established naming convention.
1673-1691: LGTM! Setup is comprehensive and includes cleanup.The test setup properly configures the environment, starts required processes, and includes cleanup in
on_exit. Good practice!apps/explorer/lib/explorer/chain/token/instance.ex (6)
11-22: Well-structured module attributes and type definitions!The new constants and type definitions are clear, properly documented, and follow Elixir conventions.
Line range hint
67-85: Good use of__MODULE__for better maintainability!The change from explicit module name to
__MODULE__makes the code more maintainable and reduces the risk of errors during module renaming.
96-102: Improved type safety with proper @SPEC annotation!The function is well-documented and the changes enhance type safety while maintaining consistent use of
__MODULE__.
881-902: Well-implemented NFT collection refetch functionality!The new function is thoroughly documented with proper @doc and @SPEC annotations, and implements the core functionality required by the PR objectives.
Line range hint
645-880: Well-implemented media handling functionality!The media handling functions are well-documented, properly handle errors, and follow best practices for CDN integration.
Line range hint
1-1195: Solid implementation with good documentation and error handling!The module successfully implements the NFT collection metadata refetch functionality while maintaining good documentation coverage and proper error handling. The changes align well with the PR objectives and follow Elixir best practices.
| :timer.sleep(100) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sleep with proper test synchronization.
Using :timer.sleep(100) can make tests flaky. Consider using ExUnit's assert_receive or a similar mechanism to properly wait for async operations.
Example:
-:timer.sleep(100)
+assert_receive {:metadata_refetch_complete, _}, 1000Committable suggestion skipped: line range outside the PR's diff.
| @doc """ | ||
| Put owner address to unique token instance. If not unique, return original instance. | ||
| """ | ||
| @spec put_owner_to_token_instance(__MODULE__.t(), Token.t(), [api?]) :: __MODULE__.t() | ||
| def put_owner_to_token_instance(token_instance, token, options \\ []) | ||
|
|
||
| def put_owner_to_token_instance(%__MODULE__{is_unique: nil} = token_instance, token, options) do | ||
| put_owner_to_token_instance(__MODULE__.put_is_unique(token_instance, token, options), token, options) | ||
| end | ||
|
|
||
| def put_owner_to_token_instance( | ||
| %__MODULE__{owner: nil, is_unique: true} = token_instance, | ||
| %Token{type: type}, | ||
| options | ||
| ) | ||
| when type in ["ERC-1155", "ERC-404"] do | ||
| owner_address_hash = | ||
| token_instance | ||
| |> __MODULE__.owner_query() | ||
| |> Chain.select_repo(options).one() | ||
|
|
||
| owner = | ||
| Address.get( | ||
| owner_address_hash, | ||
| options | ||
| |> Keyword.merge( | ||
| necessity_by_association: %{ | ||
| :names => :optional, | ||
| :smart_contract => :optional, | ||
| Implementation.proxy_implementations_association() => :optional | ||
| } | ||
| ) | ||
| ) | ||
|
|
||
| %{token_instance | owner: owner} | ||
| end | ||
|
|
||
| def put_owner_to_token_instance(%__MODULE__{} = token_instance, _token, _options), do: token_instance |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database queries.
The owner query operations could fail due to database errors. Consider adding proper error handling:
def put_owner_to_token_instance(
%__MODULE__{owner: nil, is_unique: true} = token_instance,
%Token{type: type},
options
)
when type in ["ERC-1155", "ERC-404"] do
- owner_address_hash =
- token_instance
- |> __MODULE__.owner_query()
- |> Chain.select_repo(options).one()
+ with {:ok, owner_address_hash} <-
+ token_instance
+ |> __MODULE__.owner_query()
+ |> Chain.select_repo(options).one()
+ |> case do
+ nil -> {:error, :owner_not_found}
+ address_hash -> {:ok, address_hash}
+ end,
+ {:ok, owner} <-
+ Address.get(
+ owner_address_hash,
+ options
+ |> Keyword.merge(
+ necessity_by_association: %{
+ :names => :optional,
+ :smart_contract => :optional,
+ Implementation.proxy_implementations_association() => :optional
+ }
+ )
+ ) do
+ %{token_instance | owner: owner}
+ else
+ {:error, _reason} -> token_instance
+ endCommittable suggestion skipped: line range outside the PR's diff.
Resolves #10520
Motivation
The Admin API endpoint to re-fetch NFT collection metadata is added.
Changelog
Docs update PR blockscout/docs#362.
This pull request includes significant refactoring and improvements to the
BlockScoutWebapplication, particularly focusing on the handling of token instances. The main changes involve moving several functions from theChainmodule to theInstancemodule, and adding a new endpoint for triggering NFT collection metadata refetch. Additionally, there are updates to the tests to cover the new functionality.A new endpoint for triggering NFT collection metadata refetch:
As every admin-related endpoint, requires API key to be sent in the body of the request:
Refactoring and Code Simplification:
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex: Moved functions related to token instances fromChaintoInstanceand updated corresponding calls. [1] [2] [3] [4] [5]apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/holder_controller.ex: Updated calls to useInstancemodule for token instance functions. [1] [2]apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/metadata_controller.ex: Updated calls to useInstancemodule for token instance functions. [1] [2]apps/block_scout_web/lib/block_scout_web/controllers/tokens/instance/transfer_controller.ex: Updated calls to useInstancemodule for token instance functions. [1] [2]apps/block_scout_web/lib/block_scout_web/controllers/tokens/inventory_controller.ex: Updated calls to useInstancemodule for token instance functions. [1] [2]New Functionality:
apps/block_scout_web/lib/block_scout_web/controllers/api/v2/token_controller.ex: Added a new functiontrigger_nft_collection_metadata_refetchto trigger metadata refetch for NFT collections.apps/block_scout_web/lib/block_scout_web/routers/tokens_api_v2_router.ex: Added a new route for triggering NFT collection metadata refetch.Test Updates:
apps/block_scout_web/test/block_scout_web/controllers/api/v2/token_controller_test.exs: Added tests for the newtrigger_nft_collection_metadata_refetchendpoint. [1] [2]Removal of Deprecated Code:
apps/explorer/lib/explorer/chain.ex: Removed functions related to token instance metadata fetching and updating that were moved to theInstancemodule. [1] [2] [3] [4]Checklist for your Pull Request (PR)
masterin the Version column. Changes will be reflected in this table: https://docs.blockscout.com/for-developers/information-and-settings/env-variables. The token instance re-fetcher env variables docs#362Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Performance