Skip to content

Conversation

@Vinsho
Copy link
Contributor

@Vinsho Vinsho commented Feb 3, 2026

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds chain attribution for DeBank “apps” parsing and switches deposit token parsing to a typed token model so downstream consumers can reliably access token fields.

Changes:

  • Introduced DEBANK_APP_CHAIN_MAP and used it to populate a chain: Blockchain field on parsed DebankPrediction and DebankAppDeposit.
  • Replaced raw dict token handling for app deposits with a new typed DebankDepositToken model and updated parsing to use asset_token_list.
  • Updated app parser tests to assert chain and typed token access.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
blockapi/v2/models.py Adds DebankDepositToken, types app deposit tokens, and adds chain to app-level parsed models.
blockapi/v2/api/debank_maps.py Adds an app-id → chain mapping used during app parsing.
blockapi/v2/api/debank.py Threads chain through app parsing into deposits and predictions; switches deposit token extraction to asset_token_list.
blockapi/test/v2/api/debank/test_debank_fetch.py Adds an integration test stub for fetching apps (but currently not asserting).
blockapi/test/v2/api/debank/test_debank_app_parser.py Extends parser tests to assert chain and typed token access.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1221 to 1224
position_index: str
asset_dict: Optional[dict] = None
asset_token_list: Optional[list[dict]] = None
asset_token_list: list[DebankDepositToken] = []
update_at: Optional[float] = None
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

asset_token_list uses a mutable default (=[]) on a Pydantic model. This can lead to shared state between instances and unexpected cross-test/request contamination. Prefer Field(default_factory=list) (or make it Optional[...] = None and normalize at use sites).

Copilot uses AI. Check for mistakes.
Comment on lines 35 to 36
def test_fetch_usage():
assert False
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

test_fetch_usage contains assert False, which will always fail when integration tests are enabled. Either implement the test (assert on the response / save fixture data) or mark it as skipped/xfail so it doesn’t break CI runs that include integration tests.

Suggested change
def test_fetch_usage():
assert False
@pytest.mark.skip(reason="Pending implementation of fetch_usage integration test")
def test_fetch_usage():
pass

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +41
@pytest.mark.integration
def test_fetch_debank_apps(real_debank_api):
response = real_debank_api.fetch_debank_apps('0x807A2E2e469df84b299Da5f90f15DdA4380dAcA1')
pass
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

test_fetch_debank_apps currently discards the fetched response and just passes, so it won’t detect regressions and may still hit the external API. Add at least a minimal assertion on the returned payload (or reuse _save(...) like the other integration tests), or remove the test until it has meaningful checks.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants