Feature: Implement missing PyAleph API endpoints#276
Conversation
get_address_stats
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The PR cleanly adds three new endpoints (address stats, account files, chain balances) with corresponding filters and response models, following the existing code style well. There are no security issues or critical bugs, but there are several quality issues: a dead-code condition in a fixture that obscures intent, test assertions that don't actually validate filter behavior, a misplaced enum that creates a backwards import dependency between responses and filters, and minor inconsistencies in null-checking and model strictness.
tests/unit/conftest.py (line 192): if int(1) == 1: is always True — this is dead code. The intent was probably to conditionally build data_dict based on page, but page isn't in scope here (it's the lambda parameter below). The block should simply be removed and the dict built unconditionally, since the lambda already handles the page guard on line 206. As written, the condition is misleading and makes the fixture look like it supports multi-page behaviour when it doesn't.
tests/unit/test_asynchronous_get.py (line 148): test_get_address_stats passes address_contains="0xa1" as a filter but then asserts len(address_stats) == 2. Only one of the two fixture addresses (0xa1B3...) matches that prefix; the other (0x51A5...) does not. Because the mock bypasses server-side filtering, both addresses are returned, so the assertion passes — but it gives a false impression that filter behavior is being verified. The test should either (a) assert len == 1 with the filtered mock, or (b) drop the filter and clarify this is only testing the HTTP plumbing.
src/aleph/sdk/query/filters.py (line 36): FileType is a domain model concept (a classification of stored objects), not a query parameter concept. Defining it in filters.py creates a backwards import dependency: responses.py imports from filters.py, which is unusual and harder to follow. It would be cleaner in responses.py itself (since that's the only consumer) or in a shared types.py / models.py.
src/aleph/sdk/client/http.py (line 755): if not filter: should be if filter is None: (same pattern applies at lines 784 and 818). While custom class instances are truthy by default, using is None is the idiomatic and safer check, consistent with how get_balances already uses filter.as_http_params() if filter else None. It also avoids a future bug if a filter subclass ever defines __bool__.
src/aleph/sdk/query/responses.py (line 134): AddressStats uses extra="forbid", but it's unclear whether the real API response includes a total field alongside the individual type counts. The SortByMessageType enum has a TOTAL variant (implying the server tracks a total separately), and the fixture maps item["total"] → "messages". If the live API returns both messages and total fields, pydantic will raise a validation error here. Consider using extra="ignore" until the exact API shape is confirmed, or add total: int to the model.
src/aleph/sdk/query/filters.py (line 339): min_balance is documented as "must be >= 1" but there is no validation. If the API returns a 4xx for values < 1, callers will get an unhelpful HTTP error rather than a clear ValueError. Consider adding a validator or at minimum removing the constraint from the doc if it isn't actually enforced.
17a8b0f to
ebd24a6
Compare
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR adds useful API endpoints for address stats, account files, and chain balances. However, there are several issues that need to be addressed: (1) Merge conflict markers are present in the http.py file that need to be resolved, (2) A typo in conftest.py causes the address stats fixture to always return page 1 data regardless of the page parameter, (3) A typo in test_asynchronous_get.py prevents running tests directly, and (4) The AccountFilesFilter uses inconsistent parameter naming with the API (sort_order vs sortOrder).
src/aleph/sdk/client/http.py (line 57): Merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) are present in the imports section. These need to be resolved before merging. The correct import should include AccountFilesFilter and ChainBalancesFilter from ..query.filters.
tests/unit/conftest.py (line 192): Bug: if int(1) == 1: should be if int(page) == 1:. The current code always returns the data for all pages because it's comparing 1 == 1 instead of checking the actual page parameter. This breaks pagination testing.
tests/unit/test_asynchronous_get.py (line 322): Typo: if __name__ == "__main __": has an extra space. Should be if __name__ == "__main__":. This prevents the test file from running when invoked directly with python test_asynchronous_get.py.
src/aleph/sdk/query/filters.py (line 319): Inconsistent parameter naming: sort_order should be sortOrder to match the camelCase convention used in other filters (e.g., AddressesFilter uses sortOrder on line 283). The API likely expects sortOrder.
src/aleph/sdk/client/http.py (line 989): Potential security issue: The address parameter is directly interpolated into the URL path without validation. Consider adding input validation to prevent path traversal or injection attacks. For example, validate that the address matches expected address formats for the supported chains.
src/aleph/sdk/query/responses.py (line 145): The AddressStats model uses extra='forbid' which will reject any extra fields from the API. The test fixture maps total to messages, but verify that the actual API response structure matches this model. If the API returns total instead of messages, this will cause validation errors.
- Move FileType to responses.py (where it's used) - Align query params to camelCase: sortOrder, minBalance - Validate min_balance >= 1 in ChainBalancesFilter constructor - Validate address against path traversal in get_account_files - Extract _paginated_params helper to dedupe pagination boilerplate - Standardize page_size default to 100 across new endpoints - Drop extra="forbid" from outer paginated response models - Document AddressStatsResponse.data dict shape - Fix dead int(1) == 1 check in raw_address_stats_response fixtures - Add tests for filter as_http_params() output and address validation
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The PR adds useful functionality for address stats, account files, and chain balances queries. The code is generally well-structured with good test coverage. However, there are a few issues that need attention: (1) The AddressStats model uses messages field but the API likely returns total - the test fixture masks this by converting the field, suggesting a potential model/API mismatch. (2) The path traversal protection in get_account_files doesn't catch ./ prefixes. (3) Missing validation for empty addresses in get_account_files.
src/aleph/sdk/query/responses.py (line 156): The AddressStats model uses messages field, but the test fixture in conftest.py:193 converts total to messages, suggesting the API returns total. Verify the actual API response format and update the model field name accordingly. If the API returns total, the model should use total instead of messages.
src/aleph/sdk/client/http.py (line 969): The path traversal check doesn't catch addresses starting with ./ (e.g., ./etc/passwd). Consider adding address.startswith('./') to the validation condition to catch this case.
src/aleph/sdk/client/http.py (line 969): Consider adding validation for empty or whitespace-only addresses: if not address or not address.strip(): raise ValueError(...) before the path traversal check.
tests/unit/test_asynchronous_get.py (line 368): The path traversal test doesn't cover ./ prefix cases. Add './etc/passwd' to the test cases to ensure the fix for comment #2 is covered.
src/aleph/sdk/client/http.py (line 895): Minor: The _paginated_params method uses Optional[Any] for the filter type hint. Consider using a more specific type like Optional[object] or adding a type comment for better type safety.
src/aleph/sdk/query/filters.py (line 245): Good use of the _drop_empty helper function - it simplifies the as_http_params methods and reduces code duplication across filter classes.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR successfully adds three new API endpoints (get_address_stats, get_chain_balances, get_account_files) with proper filtering, validation, and comprehensive testing. The code follows existing patterns in the codebase, includes appropriate security validation for path traversal attacks in get_account_files, and uses well-designed Pydantic models. The implementation is clean and well-tested.
src/aleph/sdk/client/http.py (line 970): The path traversal validation in get_account_files is good, but might be overly restrictive. The check .. in address will reject addresses like ..foo which aren't actually path traversal attempts. Consider using a more precise check like address.startswith('..') or '/..' in address or '\\..\\' in address to only block actual traversal patterns.
src/aleph/sdk/client/http.py (line 972): Consider also validating against URL-encoded path separators (e.g., %2F, %5C) to prevent bypass attempts through URL encoding, though aiohttp may handle this automatically.
tests/unit/test_asynchronous_get.py (line 394): Minor nit: There's a typo in the if __name__ guard - it should be "__main__" not "__main __" (extra space). This won't affect pytest but would prevent running tests directly with python test_asynchronous_get.py.
src/aleph/sdk/client/http.py (line 895): The new _paginated_params helper method is a nice refactoring that reduces code duplication. Consider adding a type annotation for the filter parameter instead of Optional[Any] - perhaps Optional[BaseFilter] if a common base class exists, or use @overload for the three filter types.
This pull request adds support for querying address statistics from the Aleph node API, including filtering and sorting capabilities. It introduces new models and filters for handling address stats, updates the HTTP client to support these queries, and adds comprehensive unit tests for the new functionality.
Address stats query feature:
AddressesFilterclass tosrc/aleph/sdk/query/filters.pyfor filtering and sorting address stats queries, including options for substring filtering and sorting by message type and order.SortByMessageTypeenum to specify supported sort fields in address stats queries.AddressStatsandAddressStatsResponsemodels insrc/aleph/sdk/query/responses.pyto represent address statistics and the paginated response structure.src/aleph/sdk/client/http.pyto add theget_address_statsmethod, allowing asynchronous retrieval of address statistics with filtering and pagination support. [1] [2]Testing and validation:
tests/unit/test_asynchronous_get.pyto verify the new address stats query functionality, including tests for filtered and unfiltered queries, and introduced fixtures intests/unit/conftest.pyto provide mock address stats data and responses. [1] [2] [3]