PE-8702: feat: optimize D2N bundle uploads with tiered atomic bundling#2093
PE-8702: feat: optimize D2N bundle uploads with tiered atomic bundling#2093vilenarios wants to merge 5 commits intodevfrom
Conversation
Implements tiered bundling strategy to dramatically reduce bundle count: - Files < 500MB: Batch together (up to 500 files or 500MB per bundle) - Files 500MB-20GB: Individual atomic bundles with streaming - Uses First-Fit-Decreasing packing for 10-20% better space utilization Benefits: - 500x reduction in bundle count for typical uploads (1000 files → 1-2 bundles) - Guaranteed atomicity (no ghost files or wasted money) - Memory efficient via streaming (handles 20GB files safely) - Fixed thumbnail encryption for private drives Changes: - Added FirstFitDecreasingBundlePacker for optimal bin packing - Updated limits (20GB max file size, 500MB batch threshold) - Routed all D2N uploads through uploadEntities for unified bundling - Implemented tiered bundling logic in data_bundler - Added CLAUDE.md for future development reference 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughUpdates upload limits and bundling strategy: introduces First-Fit-Decreasing packer, batches small files (<500 MiB) while emitting atomic bundles for large files, threads a dynamic D2N gateway callback through uploader factories, adds thumbnail per-file encryption handling, and wires contextual signing (onSign/context) across wallet/signing surfaces; adds CLAUDE.md docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Repo as UploadRepository
participant Bundler as DataBundler
participant Packer as FFD_Packer
participant Uploader as ArDriveUploader
User->>Repo: request upload(method, files)
alt method == AR (d2n)
Repo->>Bundler: prepareDataItems(files, skipThumbnailSigning=true)
Bundler->>Bundler: partition small (<500MiB) / large (>=500MiB)
Bundler->>Packer: packItems(small files)
Packer-->>Bundler: bundles (First‑Fit‑Decreasing)
Bundler-->>Repo: DataResultWithContents[] (batch bundles + atomic bundles)
Repo->>Uploader: uploadEntities(entities..., type=d2n)
else method == Turbo
Repo->>Uploader: uploadFiles(files..., type=turbo)
end
Uploader-->>User: progress / completion events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/core/upload/domain/repository/upload_repository.dart (1)
158-160: Consider simplifying the entity mapping.The explicit mapping from
(ARFSUploadMetadataArgs, IOFile)to(ARFSUploadMetadataArgs, IOEntity)appears redundant sinceIOFilealready implements/extendsIOEntity. The list could be passed directly unless there's a type variance concern.Apply this diff to simplify:
- final entities = uploadFiles - .map<(ARFSUploadMetadataArgs, IOEntity)>((f) => (f.$1, f.$2)) - .toList(); - uploadController = await _ardriveUploader.uploadEntities( - entities: entities, + entities: uploadFiles, wallet: _auth.currentUser.wallet,CLAUDE.md (1)
122-126: Address markdown linting issues for consistency.Static analysis flagged a few minor markdown formatting issues:
- Lines 122, 174, 193: Fenced code blocks should specify a language (use ```text or ```plaintext for pseudo-code)
- Line 327: Bare URL should use link syntax:
[ArFS Specification](https://github.com/...)These don't affect functionality but improve documentation quality.
Based on static analysis hints.
Also applies to: 174-185, 193-199, 327-327
lib/utils/bundles/first_fit_decreasing_bundle_packer.dart (1)
49-54: Consider enriching the exception message with item details.When an item exceeds
maxBundleSize, the error message could include additional context (e.g., item index or identifier if available) to help diagnose which specific upload is problematic. This is especially helpful when processing large batches.Example enhancement:
if (item.size > maxBundleSize) { throw Exception( - 'Item size (${item.size} bytes) exceeds max bundle size ($maxBundleSize bytes). ' - 'This item cannot be bundled.', + 'Item size (${item.size} bytes) exceeds max bundle size ($maxBundleSize bytes) ' + 'at index ${sortedItems.indexOf(item)}. This item cannot be bundled.', ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(1 hunks)lib/blocs/upload/limits.dart(1 hunks)lib/blocs/upload/models/upload_plan.dart(2 hunks)lib/core/upload/domain/repository/upload_repository.dart(1 hunks)lib/utils/bundles/first_fit_decreasing_bundle_packer.dart(1 hunks)packages/ardrive_uploader/lib/src/data_bundler.dart(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
327-327: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pre-build / test-and-lint
🔇 Additional comments (10)
lib/blocs/upload/models/upload_plan.dart (1)
6-6: LGTM! Clean migration to First-Fit-Decreasing packer.The substitution of
FirstFitDecreasingBundlePackerforNextFitBundlePackeris straightforward and well-documented. The updated logging and comments clearly explain the rationale (10-20% better space utilization).Also applies to: 62-77
lib/core/upload/domain/repository/upload_repository.dart (1)
149-178: Well-structured tiered upload path with clear separation of concerns.The branching logic cleanly separates AR (D2N) bundled uploads from Turbo single-file uploads, with helpful inline documentation.
packages/ardrive_uploader/lib/src/data_bundler.dart (2)
113-128: Excellent implementation of tiered bundling strategy.The two-phase approach (small files batched, large files atomic) is well-structured and aligns perfectly with the PR objectives:
- 500 MB threshold correctly separates small from large files
- Batch limits (500 files OR 500 MB) are properly enforced
- Folder bundling is handled separately and correctly
- Logging provides good visibility into bundle creation
Also applies to: 206-311
1013-1050: Well-implemented thumbnail bundling with proper encryption.The
_createThumbnailDataItemFilehelper correctly handles encryption for private drives, including proper cipher metadata tagging. The logic mirrors the main data encryption flow, ensuring consistency.lib/utils/bundles/first_fit_decreasing_bundle_packer.dart (2)
30-96: Solid First-Fit-Decreasing implementation with correct bin-packing logic.The algorithm correctly:
- Sorts items by size descending for optimal packing
- Tries first-fit placement before creating new bundles
- Respects both size and count constraints
- Provides comprehensive logging for debugging
The edge cases (empty list, oversized items) are properly handled.
99-106: Utilization calculation is accurate.The helper correctly computes average bundle space utilization as a percentage of total available space across all bundles.
lib/blocs/upload/limits.dart (4)
25-28: LGTM! Clear threshold for the tiered bundling strategy.The
batchBundleThresholdof 500 MB correctly implements the boundary between batched and individual atomic uploads as described in the PR objectives. The naming and documentation are clear.
3-9: Verify the implementation and user-facing changes for the 20 GiB file size limit reduction.The code implementation is properly structured with validation in place through
UploadFileSizeChecker(which checks files at upload time) and the backward compatibility alias. However, verify that:
- User-facing error messages clearly communicate the new 20 GiB limit and explain the change from the previous 65 GiB
- UI flows gracefully handle file rejection before upload begins
- Documentation and help text have been updated to reflect the new limit and the rationale for atomic bundled uploads
36-37:getBundleSizeLimitcorrectly supports the tiered bundling strategy.The function appropriately returns the batch bundle size limit (500 MB for D2N, 10 GB for Turbo). The calling code in
upload_plan_utils.dartuses this limit to correctly separate small files (for batching) from large files (for individual atomic bundles). Thedata_bundler.dartconfirms that large files ≥ 500 MB are handled separately with their own bundles up to 20 GB, ensuring atomicity as intended.
14-20: Bundling changes require manual verification of 500-file bundle performance.The constants are now consistent across the codebase (maxBundleDataItemCount = maxFilesPerBundle = 500). However:
- FirstFitDecreasingBundlePacker lacks test coverage — only NextFitBundlePacker is tested in bundle_packer_test.dart
- No performance tests for 500-file bundles — cannot verify memory usage, processing time, or upload reliability with this size
- data_bundler.dart uses a local constant (line 226) independent of limits.dart, meaning two separate bundling paths exist with the same limit
The bundling logic itself is sound (dual limits: 500 files OR 500MB per bundle), but the absence of tests for FirstFitDecreasingBundlePacker and stress testing with 500-file bundles means the performance impact of the increased limit remains unverified.
- Remove unused d2nMaxBundleSize constant from limits.dart - Optimize thumbnail preparation by skipping unnecessary signing in bundling context - Add skipThumbnailSigning parameter to prepareDataItems() - Reduces overhead when thumbnail will be re-signed for bundling - Update upload repository tests to mock uploadEntities instead of uploadFiles - D2N uploads now route through uploadEntities for atomic bundling - Use flexible any() matchers for better test maintainability All tests passing, flutter analyze clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ardrive_uploader/lib/src/data_bundler.dart (2)
113-128: Consider using thebatchBundleThresholdconstant from limits.dart.The hardcoded
batchThreshold = 500 * 1024 * 1024duplicates thebatchBundleThresholdconstant defined inlib/blocs/upload/limits.dart. This creates a maintenance risk if the threshold needs to change.Consider importing and using the constant directly:
+import 'package:ardrive/blocs/upload/limits.dart'; + // Tiered file preparations: // - Small files (< 500MB): batch together // - Large files (>= 500MB): individual bundles -const batchThreshold = 500 * 1024 * 1024; // 500 MB +final batchThreshold = batchBundleThreshold;
226-292: Edge case: First file in a batch can exceed maxBatchSize.The batching logic checks if adding a file would exceed limits (lines 239-240) but only creates a bundle if
currentBatchItems.isNotEmpty(line 242). This means the first file in a batch is always added regardless of size.While files are tiered at 500MB (
batchThreshold), a file just under 500MB (e.g., 499MB) could be the first in a batch, resulting in a bundle exceedingmaxBatchSize(500MB).This is likely acceptable since files < 500MB are by definition "small files" and the limit is a soft guideline. However, consider either:
- Documenting this as expected behavior, or
- Checking individual file size against
maxBatchSizebefore adding to a batchIf strict enforcement is needed, add a check:
for (var filePrep in smallFilePreparations) { + // Single file exceeds batch limit - still add but as solo batch + final isSoloOversize = currentBatchItems.isEmpty && + filePrep.totalSize > maxBatchSize; + // Check if adding this file would exceed limits final wouldExceedSize = currentBatchSize + filePrep.totalSize > maxBatchSize; final wouldExceedCount = currentBatchFileCount >= maxFilesPerBundle; - if ((wouldExceedSize || wouldExceedCount) && currentBatchItems.isNotEmpty) { + if (((wouldExceedSize || wouldExceedCount) && currentBatchItems.isNotEmpty) || + isSoloOversize) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/blocs/upload/limits.dart(1 hunks)packages/ardrive_uploader/lib/src/data_bundler.dart(5 hunks)test/core/upload/domain/repository/upload_repository_test.dart(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pre-build / test-and-lint
🔇 Additional comments (7)
packages/ardrive_uploader/lib/src/data_bundler.dart (4)
138-145: Good fix: skipThumbnailSigning addresses redundant work.The
skipThumbnailSigning: trueparameter correctly addresses the previous review concern about redundant thumbnail preparation. By skipping the signing step inprepareDataItems, the code avoids creating an unused signedDataItemResultand instead creates only the unsignedDataItemFileneeded for bundling (lines 165-172).Based on learnings from past review comments.
819-821: LGTM: Clean parameter addition with backward compatibility.The
skipThumbnailSigningparameter is well-designed:
- Default value
falsepreserves existing behavior for non-bundling contexts- Clear documentation explaining its purpose
- Named parameter makes call sites self-documenting
1023-1060: LGTM: Well-structured helper for thumbnail bundling.The
_createThumbnailDataItemFilehelper is properly implemented:
- Reuses
_dataGeneratorfor consistent encryption handling- Correctly updates thumbnail metadata with cipher tags when encrypted
- Returns unsigned
DataItemFilesuitable for bundling- Mirrors the logic in
createDataItemForThumbnailbut without the signing overheadThis provides the precise control needed for file-specific encryption keys in bundled uploads.
154-174: No issues found—the code correctly handles thumbnail files regardless of the skipThumbnailSigning flag.The
thumbnailFileis created at line 862 (before theskipThumbnailSigningcheck at line 880) and assigned to the preparation object at line 918. If thumbnail generation fails, the catch block at line 893 logs the error and continues, leavingthumbnailFileas null. The null-check at line 154 (if (preparation.thumbnailFile != null)) properly handles both cases: when a thumbnail was successfully generated or when generation failed. This design is sound—no fix is needed.test/core/upload/domain/repository/upload_repository_test.dart (1)
62-63: LGTM: Test updates properly reflect API migration to uploadEntities.The test changes correctly reflect the migration from
uploadFilestouploadEntities:
- Fallback registration added for the new
entitiesparameter type (lines 62-63)- All mock setups updated to use
uploadEntitieswith entity-based parameters (lines 84-90, 137-143, etc.)- All verifications updated consistently across test cases
- Clear explanatory comment added (line 83)
The changes are mechanical, consistent, and complete across all test cases.
lib/blocs/upload/limits.dart (2)
3-12: LGTM: Clear constants with good documentation.The file size constants are well-defined:
maxSingleFileSize(20 GiB) aligns with PR objectives for maximum file support- Backward compatibility alias
fileSizeLimitprevents breaking changesfileSizeWarning(5 GiB) provides useful UX threshold- Comments clearly explain purpose and constraints
30-37: Past review issue resolved: d2nMaxBundleSize removed.The unused
d2nMaxBundleSizeconstant flagged in the previous review has been correctly removed. The 20GB limit for individual large file bundles is now enforced bymaxSingleFileSizewith upstream validation, as documented in the comment (lines 31-32).The current approach is clean:
- Batch bundles (small files):
d2nBatchBundleSizeLimit(500MB)- Individual file bundles (large files): validated against
maxSingleFileSize(20GB) before reaching the bundlergetBundleSizeLimit()correctly returnsd2nBatchBundleSizeLimitfor D2NBased on past review comments.
|
Visit the preview URL for this PR (updated for commit a881976): https://ardrive-web--pr2093-pe-8702-optimize-d2n-wgcrc4gp.web.app (expires Wed, 12 Nov 2025 03:29:18 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
- Add maxSingleFileSize constant (20GB) to ardrive_uploader constants - Validate large file size before creating individual bundles - Throw clear exception with max size in GB and actual size in bytes - Ensures files > 20GB are rejected early with helpful error message Addresses CodeRabbit review feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/ardrive_uploader/lib/src/constants.dart (1)
5-7: Consider declaring asconstinstead offinal.Since the value is initialized from a compile-time constant expression (
const GiB(20).size), declaring this asconst intwould enable compile-time evaluation and allow the constant to be used in const contexts.Apply this diff:
-final int maxSingleFileSize = const GiB(20).size; +const int maxSingleFileSize = GiB(20).size;packages/ardrive_uploader/lib/src/data_bundler.dart (2)
226-292: Batching logic is correct, but extract duplicate constants.The batching implementation properly enforces both size (500 MB) and count (500 files) limits, creating bundles before thresholds are exceeded.
However,
batchThreshold(line 116) andmaxBatchSize(line 229) represent the same 500 MB constant. Extract to a single named constant at the class or file level to improve maintainability:static const _batchSizeThreshold = 500 * 1024 * 1024; // 500 MB static const _maxFilesPerBundle = 500;
1032-1069: Function works correctly but duplicates logic.The
_createThumbnailDataItemFilehelper properly creates an encryptedDataItemFilefor bundled thumbnails. However, it duplicates most of the logic fromcreateDataItemForThumbnail(lines 990-1030), differing mainly in the return type.Consider extracting the common data generation and encryption logic into a shared helper to reduce duplication:
Future<(Stream<Uint8List> Function(), int, ThumbnailUploadMetadata)> _prepareThumbnailData({ required IOFile thumbnailFile, required ThumbnailUploadMetadata thumbnailMetadata, required Wallet wallet, required String fileId, SecretKey? encryptionKey, }) async { final dataGenerator = await _dataGenerator( dataStream: thumbnailFile.openReadStream, fileLength: thumbnailMetadata.size, metadataId: fileId, wallet: wallet, encryptionKey: encryptionKey, ); if (encryptionKey != null) { thumbnailMetadata.setCipherTags( cipherTag: dataGenerator.$3!, cipherIvTag: encodeBytesToBase64(dataGenerator.$2!), ); } return (dataGenerator.$1, dataGenerator.$4, thumbnailMetadata); }Then both functions can call this helper and construct their respective return types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ardrive_uploader/lib/src/constants.dart(1 hunks)packages/ardrive_uploader/lib/src/data_bundler.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pre-build / test-and-lint
🔇 Additional comments (6)
packages/ardrive_uploader/lib/src/constants.dart (1)
5-7: Manual verification of maximum file size limit update is required.The search patterns executed did not reveal additional references to the 65 GiB limit in the codebase. However, this is inconclusive. The regex patterns may not capture:
- Error or validation messages with dynamic limits referenced
- Documentation strings or comments mentioning the old limit
- Test fixtures or example code using the old value
- Different phrasing patterns (e.g., "65,536 MiB", "70 × 10^9 bytes")
Recommended verification steps:
- Search documentation and README files for "65 GiB" references
- Review error handling and validation logic that references file size limits
- Check test files and test data for hardcoded 65 GiB values
- Scan for any user-facing strings about upload constraints
packages/ardrive_uploader/lib/src/data_bundler.dart (5)
113-128: LGTM! Clear tiered preparation setup.The tiered batching structure is well-organized. The 500 MB threshold aligns with the PR objectives, and the record-based preparation tracking clearly separates metadata, data items, and size.
138-189: Well-implemented file preparation with proper thumbnail handling.The code correctly:
- Skips thumbnail signing for bundling context (addresses past review feedback)
- Derives file-specific encryption keys for private drive thumbnails
- Includes thumbnails in bundles for atomicity (3 items per file)
- Tiers files by cumulative size threshold
The size calculation at line 148 sums
dataSizefrom eachDataItemFile, which should correctly reflect encrypted sizes.
294-322: Excellent size validation implementation!The validation at lines 299-305 correctly enforces the 20GB limit before creating individual bundles for large files. The error message is clear and includes both the configured maximum and the offending file size. This properly addresses the past review feedback.
The individual bundle creation ensures atomicity for large files (500MB–20GB) as stated in the PR objectives.
829-830: Good addition for bundling optimization.The
skipThumbnailSigningparameter enables the optimization requested in past review feedback, allowing the bundling context to avoid redundant thumbnail signing while maintaining backward compatibility with the false default.
888-899: Conditional signing correctly implemented.The logic properly uses the new
skipThumbnailSigningflag to avoid redundant signing in bundling contexts while maintaining the existing signing flow when needed. The thumbnail metadata is appropriately updated in both cases.
Add ability to configure custom gateway URLs for D2N (Direct-to-Network) uploads that responds to gateway changes without requiring app restart. Changes: - Add optional gateway URL callback to ArDriveUploader factory - Update D2NStreamedUpload to accept custom Arweave client - StreamedUploadFactory dynamically creates Arweave client per upload - Fix Wallet.sign() signature in ArConnectWallet and EthereumProviderWallet to accept optional context parameter - Add gateway update logging in GarRepository - Update all ArDriveUploader instantiations to use gateway callback - Update package dependencies to use local arweave-dart for development The gateway URL is now read from Settings > Gateway configuration on each upload, allowing users to switch between gateways (arweave.net, arweave.nexus, custom URLs) without restarting the application. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
packages/arconnect/pubspec.yaml (1)
15-15: Hardcoded Windows path breaks cross-platform builds.Same issue as in other pubspec files—the absolute Windows path will fail on non-Windows systems and in CI/CD.
pubspec.yaml (1)
164-164: Hardcoded Windows path in dependency_overrides.Same issue in the dependency_overrides section—this will fail on non-Windows systems.
packages/pst/pubspec.yaml (1)
15-15: Hardcoded Windows path breaks cross-platform builds.Same critical issue—the absolute Windows path will fail on non-Windows systems.
packages/ardrive_uploader/example/pubspec.yaml (1)
38-38: Hardcoded Windows path breaks cross-platform builds.The example app also has the hardcoded Windows path, which will prevent it from building on other platforms.
packages/arconnect/lib/src/arconnect/arconnect_wallet.dart (1)
22-24: Clarify the unused optional context parameter.The
signmethod signature now includes an optionalcontextparameter that is not used in the implementation. If this is to match a base class interface but ArConnect doesn't support context, consider adding a comment explaining this.
🧹 Nitpick comments (1)
packages/ardrive_uploader/lib/src/factories.dart (1)
112-118: Consider caching Arweave client instances.The current implementation creates a fresh
Arweaveclient for each upload. While this ensures the latest gateway URL is used, it may have performance implications if uploads are frequent.Consider whether the client should be cached and reused when the gateway URL hasn't changed, or if the per-upload instantiation is intentional for ensuring up-to-date configuration.
If caching is desired, consider this approach:
// Add fields to cache client and URL Arweave? _cachedArweaveClient; String? _cachedGatewayUrl; Future<StreamedUpload> fromUploadType(UploadTask task) async { if (task.type == UploadType.d2n) { final gatewayUrl = getD2nGatewayUrl?.call(); // Reuse client if URL hasn't changed if (gatewayUrl != null) { if (_cachedGatewayUrl != gatewayUrl || _cachedArweaveClient == null) { _cachedArweaveClient = Arweave(gatewayUrl: Uri.parse(gatewayUrl)); _cachedGatewayUrl = gatewayUrl; } return D2NStreamedUpload(arweaveClient: _cachedArweaveClient); } return D2NStreamedUpload(arweaveClient: null); } // ... rest of implementation }However, if gateway URLs are expected to change frequently or the overhead is negligible, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.vscode/settings.json(1 hunks)lib/components/create_manifest_form.dart(1 hunks)lib/components/upload_form.dart(1 hunks)lib/gar/domain/repositories/gar_repository.dart(3 hunks)lib/main.dart(1 hunks)lib/services/arconnect/arconnect_wallet.dart(1 hunks)lib/services/ethereum/provider/ethereum_provider_wallet.dart(1 hunks)lib/utils/dependency_injection_utils.dart(1 hunks)packages/arconnect/lib/src/arconnect/arconnect_wallet.dart(1 hunks)packages/arconnect/pubspec.yaml(1 hunks)packages/ardrive_uploader/example/pubspec.yaml(1 hunks)packages/ardrive_uploader/lib/src/ardrive_uploader.dart(2 hunks)packages/ardrive_uploader/lib/src/d2n_streamed_upload.dart(2 hunks)packages/ardrive_uploader/lib/src/factories.dart(1 hunks)packages/ardrive_uploader/pubspec.yaml(1 hunks)packages/ardrive_utils/pubspec.yaml(1 hunks)packages/pst/pubspec.yaml(1 hunks)pubspec.yaml(2 hunks)
🔇 Additional comments (12)
.vscode/settings.json (1)
22-27: Good dev environment hygiene.The exclusions for
.fvm/(Flutter Version Manager directory) prevent unnecessary file watching and search indexing, which improves editor performance and reduces noise during development. This is a sensible configuration practice for SDK/package directories.lib/gar/domain/repositories/gar_repository.dart (2)
5-5: LGTM: Logger import added for observability.The logger import supports the new logging statements and aligns with the PR's goal of enhanced logging across gateway flows.
121-122: LGTM: Logging added for custom gateway updates.The info-level logging improves observability and is consistent with the logging added in
updateGateway. The log message is clear and helpful for debugging gateway configuration changes.packages/ardrive_uploader/lib/src/d2n_streamed_upload.dart (2)
10-12: LGTM: Clean addition of optional gateway customization.The optional
arweaveClientfield enables per-upload gateway configuration, which aligns with the PR's goal of optimizing D2N uploads.
30-36: LGTM: Proper conditional handling of custom gateway.The logging and null-safe usage of
arweaveClientis correctly implemented.lib/main.dart (1)
511-513: LGTM: Dynamic gateway URL configuration.The callback approach enables runtime gateway selection from Settings, which aligns with the PR's D2N optimization goals.
packages/ardrive_uploader/lib/src/ardrive_uploader.dart (1)
65-65: LGTM! Dynamic gateway URL provisioning added.The addition of the
getD2nGatewayUrlcallback parameter enables runtime configuration of the D2N gateway URL, which aligns well with the PR's goal of optimizing D2N bundle uploads. The parameter is properly threaded through toStreamedUploadFactory.Also applies to: 90-90
lib/services/arconnect/arconnect_wallet.dart (1)
22-24: Clarify the unused optional context parameter.The
signmethod signature now includes an optionalcontextparameter, but it's not passed toarConnectService.getSignature(message). This suggests the signature was updated to match a base class interface, but the implementation doesn't utilize the context.Is this intentional? If the context parameter is required by the
Walletbase class but not used by ArConnect, consider adding a comment explaining why it's ignored.lib/components/upload_form.dart (1)
83-83: LGTM! Gateway URL callback properly configured.The
getD2nGatewayUrlcallback is correctly configured to retrieve the D2N gateway URL fromConfigServiceat runtime, enabling dynamic gateway resolution.lib/utils/dependency_injection_utils.dart (1)
103-103: LGTM! Dependency injection properly configured.The
getD2nGatewayUrlcallback is correctly wired through the dependency injection layer, enabling centralized configuration of the D2N gateway URL.lib/components/create_manifest_form.dart (1)
73-73: LGTM! Gateway URL callback integrated in manifest creation flow.The
getD2nGatewayUrlcallback is properly integrated into the manifest creation workflow, maintaining consistency with the upload form implementation.packages/ardrive_uploader/lib/src/factories.dart (1)
101-105: LGTM! Gateway URL callback field added.The
getD2nGatewayUrlfield enables dynamic gateway URL provisioning for D2N uploads, properly integrated into the factory pattern.
| final newGatewayUrl = 'https://${gateway.settings.fqdn}'; | ||
| logger.i('Gateway updated to: $newGatewayUrl (${gateway.settings.label})'); | ||
|
|
||
| await configService.updateAppConfig( | ||
| configService.config.copyWith( | ||
| defaultArweaveGatewayForDataRequest: SelectedGateway( | ||
| label: gateway.settings.label, | ||
| url: 'https://${gateway.settings.fqdn}', | ||
| url: newGatewayUrl, |
There was a problem hiding this comment.
🧩 Analysis chain
Logging addition approved; verify hardcoded HTTPS protocol.
The URL extraction and logging improve code clarity and observability. However, the hardcoded https:// protocol (line 76) differs from custom gateways, which respect the protocol from the URL (line 156).
Please verify that all ARIO SDK gateways use HTTPS, or consider using gateway.settings.protocol instead of hardcoding.
🏁 Script executed:
#!/bin/bash
# Verify Gateway object structure and protocol field usage from ario_sdk
rg -n -C3 'gateway\.settings\.protocol' --type=dartLength of output: 550
---
🏁 Script executed:
```shell
#!/bin/bash
# Check SelectedGateway definition and its url field type
rg -n -A5 'class SelectedGateway' --type=dart
# Check how getGatewayUri is used elsewhere
echo "=== getGatewayUri usage ==="
rg -n 'getGatewayUri' --type=dart -C2
# Check if url field in SelectedGateway is used/validated elsewhere
echo "=== SelectedGateway usage in updateAppConfig ==="
rg -n 'defaultArweaveGatewayForDataRequest' --type=dart -C2
Length of output: 27307
---
🏁 Script executed:
```shell
#!/bin/bash
# Check _createCustomGateway implementation to see how custom gateways are created
rg -n -A10 '_createCustomGateway' lib/gar/domain/repositories/gar_repository.dart
# Check if any HTTP gateways are used in tests or config
echo "=== HTTP gateway patterns ==="
rg -n 'http://' lib/gar --type=dart
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Find the correct path to gar_repository.dart
echo "=== Finding gar_repository.dart ==="
fd 'gar_repository\.dart' --type f
# Once found, search for _createCustomGateway
echo "=== Searching for _createCustomGateway ==="
rg -n '_createCustomGateway' --type=dart -A10
# Check for HTTP gateway patterns in entire codebase
echo "=== HTTP gateway patterns ==="
rg -n 'http://' --type=dartLength of output: 3139
Use gateway.settings.protocol instead of hardcoding https://.
The updateGateway method hardcodes https:// on line 76, but the _createCustomGateway method (line 156) shows the code already respects gateway.settings.protocol from parsed URLs. Since the Gateway object provides settings.protocol, use it consistently:
final newGatewayUrl = '${gateway.settings.protocol}://${gateway.settings.fqdn}';
This ensures the stored URL matches the gateway's actual protocol instead of enforcing HTTPS when the gateway might specify a different scheme.
🤖 Prompt for AI Agents
In lib/gar/domain/repositories/gar_repository.dart around lines 76 to 83, the
code hardcodes "https://" when building newGatewayUrl; change it to use
gateway.settings.protocol so the stored URL reflects the gateway's actual scheme
by composing the URL from the protocol and fqdn (i.e. use
gateway.settings.protocol + '://' + gateway.settings.fqdn) and assign that to
SelectedGateway.url when calling configService.updateAppConfig.
| Future<Uint8List> sign( | ||
| Uint8List message, { | ||
| int? chainId, | ||
| }) async { | ||
| Uint8List message, [String? context] | ||
| ) async { | ||
| final signature = | ||
| await credentials.signPersonalMessage(message, chainId: chainId); | ||
| await credentials.signPersonalMessage(message); | ||
| return signature; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify breaking API change and unused context parameter.
The method signature changed from sign(Uint8List message, { int? chainId }) to sign(Uint8List message, [String? context]), which is a breaking change. Additionally, the context parameter is accepted but never used.
Run the following script to verify all call sites have been updated:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of the sign method on EthereumProviderWallet or EthereumWallet
# Search for sign method calls with chainId parameter (old usage)
echo "Searching for old sign() calls with chainId parameter..."
rg -nP --type=dart '\bsign\s*\([^)]*chainId\s*:'
# Search for sign method implementations and overrides
echo -e "\nSearching for sign method declarations..."
ast-grep --pattern $'sign($_, $_) {
$$$
}'Length of output: 99
Running broader verification to check all .sign( call sites and verify the context parameter usage:
Let me search the codebase for all .sign( calls and the file structure:
🏁 Script executed:
#!/bin/bash
# Search for all .sign( calls in dart files
echo "Searching for .sign( calls..."
rg -n --type=dart '\.sign\s*\(' -A2 -B2
echo -e "\n--- File structure for EthereumProviderWallet ---"
# Find and display the ethereum_provider_wallet.dart file structure
fd -n "ethereum_provider_wallet.dart" --exec cat {}Length of output: 14901
Let me fix the command to view the file:
🏁 Script executed:
#!/bin/bash
# Find and display ethereum_provider_wallet.dart
echo "=== Finding ethereum_provider_wallet.dart ==="
fd "ethereum_provider_wallet.dart" -type f
echo -e "\n=== File content ==="
find . -name "ethereum_provider_wallet.dart" -type f -exec cat {} \;
echo -e "\n=== Check if sign is overriding an interface method ==="
# Search for any interface or abstract class definition that might define sign
rg -B5 -A15 "abstract|interface|extends|implements" --type=dart -A2 | grep -A2 -B2 "ethereum_provider_wallet\|EthereumWallet\|sign"Length of output: 26745
Let me view the parent class to understand the method signature it expects:
🏁 Script executed:
#!/bin/bash
# View the parent EthereumWallet class to see the sign method signature
echo "=== EthereumWallet abstract class ==="
cat lib/services/ethereum/ethereum_wallet.dart | grep -A 30 "abstract class EthereumWallet"
echo -e "\n=== Search for sign method in EthereumWallet ==="
rg -A 5 "Future.*sign.*\(" lib/services/ethereum/ethereum_wallet.dartLength of output: 1219
Let me find the base Wallet class:
🏁 Script executed:
#!/bin/bash
# Find the Wallet class and its sign method
echo "=== Finding Wallet base class ==="
rg -B5 -A20 "class Wallet" lib/services/ethereum/ethereum_wallet.dart
echo -e "\n=== Searching for Wallet import and definition ==="
rg "from|import.*Wallet" lib/services/ethereum/ethereum_wallet.dart
echo -e "\n=== Search for Wallet class in arweave package ==="
rg -A 10 "abstract.*class Wallet|class Wallet.*{" --type=dart | head -50Length of output: 446
🏁 Script executed:
#!/bin/bash
# View the full ethereum_wallet.dart file to see imports
echo "=== Full ethereum_wallet.dart ==="
cat lib/services/ethereum/ethereum_wallet.dart
echo -e "\n=== Check for Wallet in arweave imports ==="
rg "import.*arweave" lib/services/ethereum/ethereum_wallet.dartLength of output: 1562
Remove unused context parameter and fix stale comment.
The breaking API change has been successfully migrated—all old chainId usages are gone and callers have been updated. However, the new context parameter is accepted but never used in the implementation or by any caller. This should either be removed or documented. Additionally, the comment on line 16 mentions "chainId" but the parameter is now "context"—update or remove it.
🤖 Prompt for AI Agents
In lib/services/ethereum/provider/ethereum_provider_wallet.dart around lines 17
to 23, the sign method accepts an unused optional parameter `context` (and the
surrounding comment still references the old `chainId`), so remove the unused
`context` parameter from the method signature and call sites, and update or
delete the stale comment to reflect the current API; ensure the method becomes
Future<Uint8List> sign(Uint8List message) async { ... } and adjust any doc
comment to no longer mention chainId/context.
- Add onSign callback support to all wallet classes (ArConnectWallet, EthereumProviderWallet, EthereumWallet) for signature logging - Create centralized walletOnSign callback in logger.dart - Pass onSign callback to all wallet instantiation points - Add context parameter to all wallet.sign() and signDataItem() calls - Add context parameter to all ArweaveSigner usages - Update arweave-dart ref to d441df4 which adds context support to Signer and ArweaveSigner classes Context strings identify signing operations: - turbo-nonce-auth, drive-key-derivation-v1/v2, eth-signature-verification - bundle-upload, file-data-upload, entity-tx, entity-data-item - drive-create, snapshot-create, pin-file, license-assertion, etc. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ardrive_crypto/lib/src/keys.dart (1)
20-34: Remove this unused function; the active implementation is inlib/core/crypto/crypto.dart.The TODO is correct—this
deriveDriveKeyfunction is dead code. All active usages in the codebase call the implementation inlib/core/crypto/crypto.dart(lines 54-67), which includes proper versioning support withDriveSignatureTypeparameter and uses context strings'drive-key-derivation-v1'and'drive-key-derivation-v2'. This simpler variant in the ardrive_crypto package should be removed.
♻️ Duplicate comments (1)
pubspec.yaml (1)
165-168: Ensure dependency_overrides consistency is maintained.Both the
dependenciesanddependency_overridessections reference the same arweave commit, which is correct for ensuring consistent resolution across all packages.Note: The commit verification requested for packages/pst/pubspec.yaml applies here as well, since all packages use the same commit reference.
🧹 Nitpick comments (2)
lib/utils/turbo_utils.dart (1)
11-16: LGTM! Context parameter added correctly.The addition of the
'turbo-nonce-auth'context string towallet.sign()aligns with the broader pattern of context-aware signing introduced across the codebase. The implementation is correct and maintains existing behavior while adding traceability.Optional: Consider extracting the context string to a constant.
If this context string might be referenced elsewhere or for consistency with other signing contexts in the codebase, consider extracting it:
🔎 Optional refactor
// At the top of the file or in a shared constants file const String kTurboNonceAuthContext = 'turbo-nonce-auth'; // Then in the function: final signature = await wallet.sign( Uint8List.fromList( (data != null ? '$data$nonce' : nonce).toString().codeUnits, ), kTurboNonceAuthContext, );lib/authentication/login/blocs/login_bloc.dart (1)
151-894: walletOnSign is properly defined and imported; consider extracting context strings into constants.The
walletOnSigncallback is correctly exported fromlib/utils/logger.dartas a getter returningwalletSignCallback, which is a simple logging callback that doesn't throw exceptions. The import is properly configured inlogin_bloc.dart.However, the hard-coded context strings ('eth-signature-verification', 'eth-wallet-verification-upload', 'eth-verification-upload', 'metamask-wallet-derive') appear only in this file. Extracting these into named constants would prevent typos and improve maintainability:
class SigningContexts { static const ethSignatureVerification = 'eth-signature-verification'; static const ethWalletVerificationUpload = 'eth-wallet-verification-upload'; static const ethVerificationUpload = 'eth-verification-upload'; static const metamaskWalletDerive = 'metamask-wallet-derive'; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
lib/authentication/login/blocs/login_bloc.dartlib/authentication/login/blocs/stub_web_wallet.dartlib/authentication/login/blocs/web_wallet.dartlib/blocs/create_snapshot/create_snapshot_cubit.dartlib/blocs/drive_create/drive_create_cubit.dartlib/blocs/fs_entry_license/fs_entry_license_bloc.dartlib/blocs/pin_file/pin_file_bloc.dartlib/blocs/upload/upload_handles/file_data_item_upload_handle.dartlib/blocs/upload/upload_handles/folder_data_item_upload_handle.dartlib/core/arfs/use_cases/upload_file_metadata.dartlib/core/arfs/use_cases/upload_folder_metadata.dartlib/core/crypto/crypto.dartlib/core/upload/bundle_signer.dartlib/core/upload/transaction_signer.dartlib/models/daos/profile_dao.dartlib/services/arconnect/arconnect_wallet.dartlib/services/arweave/arweave_service.dartlib/services/ethereum/ethereum_wallet.dartlib/services/ethereum/provider/ethereum_provider_wallet.dartlib/services/ethereum/provider/implementations/ethereum_provider_web.dartlib/shared/blocs/private_drive_migration/private_drive_migration_bloc.dartlib/utils/logger.dartlib/utils/turbo_utils.dartpackages/arconnect/lib/src/arconnect/arconnect_wallet.dartpackages/arconnect/pubspec.yamlpackages/ardrive_crypto/lib/src/keys.dartpackages/ardrive_crypto/pubspec.yamlpackages/ardrive_uploader/example/pubspec.yamlpackages/ardrive_uploader/pubspec.yamlpackages/ardrive_utils/lib/src/sign_nounce_and_data.dartpackages/ardrive_utils/pubspec.yamlpackages/pst/pubspec.yamlpubspec.yamltest/blocs/create_snapshot_cubit_test.darttest/test_utils/utils.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ardrive_uploader/example/pubspec.yaml
- packages/ardrive_uploader/pubspec.yaml
- packages/ardrive_utils/pubspec.yaml
🔇 Additional comments (37)
packages/arconnect/pubspec.yaml (1)
17-17: Dependency update is consistent across packages.The arweave dependency update aligns with the repo-wide migration to commit d441df4eea1f432051d4871b6a369d743c72f0b5, ensuring consistent behavior across all packages.
packages/ardrive_crypto/pubspec.yaml (1)
16-16: LGTM!The arweave dependency update is consistent with the broader repository changes to support context-aware signing.
pubspec.yaml (1)
54-56: Past Windows path issue is resolved.The hardcoded Windows path mentioned in the previous review has been replaced with a proper git dependency. The arweave dependency now correctly references the GitHub repository.
packages/pst/pubspec.yaml (1)
17-17: Arweave-dart commit verified and contains expected context-aware signing functionality.The commit d441df4eea1f432051d4871b6a369d743c72f0b5 exists and introduces the context parameter to Signer and ArweaveSigner as intended. The implementation adds a context field for default signing context and passes it through to wallet.sign() for logging/tracking—aligning with the PR objectives. The changes are focused and well-documented.
packages/ardrive_utils/lib/src/sign_nounce_and_data.dart (1)
12-17: Context parameter added for Turbo nonce signing.The addition of
'turbo-nonce-auth'as the context parameter aligns with the PR's implementation of context-aware signing across the codebase. The arweave package version in pubspec.yaml correctly references commit d441df4, which supports the context parameter inWallet.sign(). The context string naming follows the established kebab-case convention used consistently throughout the codebase (e.g., 'drive-key-derivation', 'drive-migration-v1-signature', 'eth-signature-verification').lib/blocs/fs_entry_license/fs_entry_license_bloc.dart (1)
255-255: LGTM! Context-aware signing added for license assertions.The addition of
context: 'license-assertion'to the ArweaveSigner instantiation improves observability by identifying the signing operation's purpose. This aligns with the repository-wide pattern of contextual signing.lib/blocs/drive_create/drive_create_cubit.dart (1)
122-122: LGTM! Context-aware signing added for drive creation.The addition of
context: 'drive-create'enhances traceability for drive and root folder entity signing operations.lib/blocs/pin_file/pin_file_bloc.dart (1)
285-285: LGTM! Context-aware signing added for file pinning.The addition of
context: 'pin-file'provides clear tracing for pin file operations.lib/services/arweave/arweave_service.dart (5)
1249-1249: LGTM! Context-aware signing added for entity transactions.The addition of
context: 'entity-tx'improves observability for entity transaction signing.
1269-1269: LGTM! Context-aware signing added for entity data items.The addition of
context: 'entity-data-item'provides clear tracing for data item signing operations.
1291-1291: LGTM! Context-aware signing added for data bundle transactions.The addition of
context: 'data-bundle-tx'enables tracing of bundle transaction signing.
1310-1310: LGTM! Context-aware signing added for bundled data items.The addition of
context: 'bundled-data-item'provides clear identification for nested bundle signing operations.
1329-1329: LGTM! Context-aware signing added for blob-based bundle transactions.The addition of
context: 'data-bundle-tx-blob'distinguishes blob-based bundle signing from other bundle operations.lib/services/arconnect/arconnect_wallet.dart (1)
8-10: LGTM! Callback-based signing observation added.The addition of the
onSigncallback and optionalcontextparameter enables external observers to trace signing operations. The logging provides helpful debugging information without exposing sensitive data.Also applies to: 25-27, 32-36
packages/arconnect/lib/src/arconnect/arconnect_wallet.dart (1)
6-8: LGTM! Signing observation added to ArConnect package wallet.The addition of
onSigncallback andcontextparameter mirrors the main app's implementation, maintaining consistency across the codebase. The use ofdebugPrintis appropriate for a package.Also applies to: 23-25
lib/blocs/create_snapshot/create_snapshot_cubit.dart (1)
303-303: LGTM! Context-aware signing added for snapshot creation.The addition of
context: 'snapshot-create'enables tracing of snapshot transaction signing operations.lib/core/upload/transaction_signer.dart (1)
82-82: LGTM! Context-aware signing added for file data uploads.The addition of
context: 'file-data-upload'provides clear identification for file data transaction signing, improving observability across upload workflows.lib/blocs/upload/upload_handles/folder_data_item_upload_handle.dart (1)
69-71: LGTM!The context parameter
'folder-entity-upload'accurately describes the signing operation and aligns with the project-wide pattern of contextual signing being introduced in this PR.test/test_utils/utils.dart (1)
250-251: LGTM!The test utility correctly uses a descriptive context
'test-data-item'for signing, maintaining consistency with the contextual signing pattern throughout the codebase.lib/blocs/upload/upload_handles/file_data_item_upload_handle.dart (1)
74-74: LGTM!The context
'file-data-item-upload'appropriately describes the signing operation for both the data transaction and entity transaction within this handle.lib/core/arfs/use_cases/upload_folder_metadata.dart (1)
62-62: LGTM!The context
'folder-metadata-upload'clearly identifies the purpose of signing operations in this use case, covering both the Turbo (DataItem) and Arweave (Transaction) upload paths.lib/core/crypto/crypto.dart (1)
76-84: LGTM!Good use of versioned context strings (
'drive-key-derivation-v1'and'drive-key-derivation-v2') that correspond to theDriveSignatureType. This provides clear traceability for debugging and auditing which signature scheme was used during key derivation.lib/core/upload/bundle_signer.dart (1)
72-72: LGTM!The context
'bundle-upload'appropriately identifies bundle transaction signing operations.lib/authentication/login/blocs/web_wallet.dart (1)
9-9: LGTM!The addition of the
onSign: walletOnSigncallback enables consistent signing event observation across wallet operations. The debug log provides useful traceability for wallet generation.test/blocs/create_snapshot_cubit_test.dart (1)
35-35: LGTM! Context strings are clear and appropriate for test scenarios.The test helper functions now pass meaningful context strings (
'test-snapshot-tx'and'test-snapshot-data-item') toArweaveSigner, which aligns with the broader PR objective of enabling context-aware signing for improved traceability.Also applies to: 47-47
lib/utils/logger.dart (1)
4-4: LGTM! Centralized signing callback enables consistent logging across wallet operations.The
walletSignCallbackfunction andwalletOnSigngetter provide a clean, reusable pattern for logging wallet signing operations with optional context. This aligns well with the PR's goal of improving observability for signing operations across various wallet types.Also applies to: 13-20
lib/core/arfs/use_cases/upload_file_metadata.dart (1)
61-61: LGTM! Context string is descriptive and appropriate.The
'file-metadata-upload'context clearly identifies the signing purpose for file metadata uploads, improving traceability and debugging.lib/services/ethereum/provider/implementations/ethereum_provider_web.dart (1)
44-45: LGTM! Signing callback wiring enables consistent logging for Ethereum provider wallets.The addition of the connection log and
onSigncallback ensures that Ethereum provider wallet signing operations are logged consistently with other wallet types in the codebase.lib/shared/blocs/private_drive_migration/private_drive_migration_bloc.dart (1)
110-111: LGTM! Context strings clearly distinguish different signing operations in drive migration.The two distinct context strings (
'drive-migration-v1-signature'and'drive-migration-signature') appropriately differentiate between signing the message for v1 signature derivation and signing the data item for upload, improving traceability in this critical migration flow.Also applies to: 132-132
lib/authentication/login/blocs/stub_web_wallet.dart (1)
4-4: LGTM! Stub wallet implementation now includes consistent signing callback wiring.The logging message and
onSigncallback ensure that even the stub wallet implementation (used for testing/dev) follows the same signing traceability pattern as production code.Also applies to: 35-36
lib/models/daos/profile_dao.dart (1)
65-65: LGTM! Consistent signing callback wiring across both wallet loading paths.The logging messages clearly distinguish between JSON and ArConnect wallet loading paths, and the
onSigncallback is correctly wired for bothWallet.fromJwkandArConnectWalletconstructors. This ensures consistent signing traceability regardless of profile type.Also applies to: 68-68, 76-76, 79-79
lib/services/ethereum/provider/ethereum_provider_wallet.dart (1)
14-16: Context parameter is properly utilized with meaningful values at call sites.Verified that the single call site in
lib/authentication/login/blocs/login_bloc.dart:886passes a meaningful context string'metamask-wallet-derive'to thesign()method, and the implementation correctly propagates it to theonSigncallback and logging.lib/authentication/login/blocs/login_bloc.dart (5)
151-155: Consistent addition of signing callback.The
onSign: walletOnSignparameter is correctly threaded through theWallet.fromJwkconstructor. This enables context-aware logging for wallet operations.
344-345: Context parameter added for signature verification.The context string
'eth-signature-verification'provides useful debugging context. The implementation is correct.
547-547: ArweaveSigner context parameter added correctly.The
context: 'eth-verification-upload'parameter is correctly passed toArweaveSigner, aligning with the updated arweave-dart library that supports contextual signing.
626-627: ArConnect wallet wired with signing callback.The
onSign: walletOnSignparameter is correctly added to theArConnectWalletconstructor, maintaining consistency across wallet types.
893-894: EthereumProviderWallet signing callback configured.The
onSign: walletOnSignparameter completes the signing callback integration across all wallet types used in the login flow.
| import 'package:cryptography/cryptography.dart'; | ||
|
|
||
| abstract class EthereumWallet extends Wallet { | ||
| EthereumWallet({super.onSign}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find and examine the Wallet class definition
echo "=== Locating Wallet class ==="
fd -t f "\.dart$" | xargs grep -l "class Wallet" | head -5
# Step 2: Check Wallet class constructor
echo -e "\n=== Checking Wallet class constructor ==="
fd -t f "wallet\.dart$" -o "Wallet.dart$" | head -1 | xargs cat -n | head -30
# Step 3: Find EthereumWallet definition for context
echo -e "\n=== Checking EthereumWallet file ==="
cat -n lib/services/ethereum/ethereum_wallet.dart
# Step 4: Find subclasses of EthereumWallet
echo -e "\n=== Finding EthereumWallet subclasses ==="
rg "extends EthereumWallet|implements EthereumWallet" --type dart -n
# Step 5: Check for test files related to EthereumWallet or onSign
echo -e "\n=== Finding test files ==="
fd -t f "test" -o "*_test.dart" | grep -i ethereumRepository: ardriveapp/ardrive-web
Length of output: 2561
🏁 Script executed:
#!/bin/bash
# Check EthereumProviderWallet implementation
echo "=== Checking EthereumProviderWallet constructor ==="
cat -n lib/services/ethereum/provider/ethereum_provider_wallet.dart | head -40
# Search for test files
echo -e "\n=== Finding test files ==="
find . -name "*test.dart" -type f | grep -i wallet | head -10
# Search for any usage of onSign parameter in the codebase
echo -e "\n=== Searching for onSign usage ==="
rg "onSign" --type dart -B 2 -A 2 | head -50
# Check pubspec to understand arweave dependency
echo -e "\n=== Checking pubspec dependencies ==="
rg "arweave" pubspec.yaml -A 2Repository: ardriveapp/ardrive-web
Length of output: 6178
🏁 Script executed:
#!/bin/bash
# Search for wallet-related test files
echo "=== Finding wallet test files ==="
find . -name "*test.dart" -type f | xargs grep -l "Wallet\|wallet" | grep test
# Check if there are tests for ethereum wallet specifically
echo -e "\n=== Checking for ethereum wallet tests ==="
find . -name "*test.dart" -type f | xargs grep -l "Ethereum" 2>/dev/null
# Look for any test that uses onSign
echo -e "\n=== Checking for onSign tests ==="
rg "onSign" --type dart | grep -i testRepository: ardriveapp/ardrive-web
Length of output: 1354
🏁 Script executed:
#!/bin/bash
# Check login_bloc_test for ethereum wallet and onSign testing
echo "=== Checking login_bloc_test.dart for ethereum wallet tests ==="
cat -n ./test/authentication/login/blocs/login_bloc_test.dart | grep -A 10 -B 2 -i ethereumRepository: ardriveapp/ardrive-web
Length of output: 4203
Add test coverage for the onSign callback parameter.
The constructor correctly forwards the optional onSign parameter to the superclass using Dart's super parameter syntax, and EthereumProviderWallet properly implements this with active callback invocation in the sign method. However, no dedicated unit tests exist for the onSign callback scenarios. Add test cases that exercise both paths: with and without the onSign callback passed to the constructor.
🤖 Prompt for AI Agents
In lib/services/ethereum/ethereum_wallet.dart around line 12, add unit tests
that cover the optional onSign constructor parameter: create tests that
instantiate EthereumWallet (or its concrete EthereumProviderWallet) without
passing onSign and assert sign behavior proceeds normally without invoking a
callback, and another test that supplies a mock/spy onSign callback, performs a
sign operation, and asserts the callback was invoked with expected arguments and
any returned/side-effectual behavior is respected; ensure setup includes any
required mocks for provider/network and tear down/reset of the spy between
tests.
| logger.d('EthereumProviderWallet instantiated'); | ||
| } | ||
|
|
||
| // Ethereum Provider accepts an optional chainId parameter |
There was a problem hiding this comment.
Update stale comment referencing removed chainId parameter.
Line 18 references "Ethereum Provider accepts an optional chainId parameter," but the signature was changed to accept context instead. Update the comment to reflect the current API.
🔎 Proposed fix
- // Ethereum Provider accepts an optional chainId parameter
+ // Context parameter enables signing operation traceability🤖 Prompt for AI Agents
In lib/services/ethereum/provider/ethereum_provider_wallet.dart around line 18,
the comment incorrectly mentions a removed optional `chainId` parameter; update
the comment to state that the Ethereum Provider accepts a `context` parameter
(or other current parameter name) instead and briefly describe its purpose
(e.g., providing request/contextual info for provider initialization), ensuring
the comment matches the current method signature and intent.
1 participant
Implements tiered bundling strategy to dramatically reduce bundle count:
Benefits:
Changes:
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
New Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.