Skip to content

Conversation

@parkan
Copy link
Collaborator

@parkan parkan commented Jul 15, 2025

This bundles together two major enhancements:

  • feat: allow importing wallets by actor id for tracking of non-owned wallets, for tracking 3rd party deals or insecure environments
  • perf: improve dealtracker performance ~25x at comparable CPU and RAM usage, resulting in <10 minute runtime n testing

Please see the two individual commits for details

@parkan parkan force-pushed the feat/standalone-dealtracker branch from c904ff9 to 697782c Compare July 15, 2025 13:57
@parkan
Copy link
Collaborator Author

parkan commented Jul 15, 2025

ok, I see we ended up with conflicts, this is one downside to the new migrations strategy, resolved

I also noticed that the CI may not have been updated to use the new migrations system (go generate is failing)

staticcheck seems to be having issues again as well, though I really hope this is just a GH actions fluke (checks pass locally)

@parkan
Copy link
Collaborator Author

parkan commented Jul 15, 2025

ok we've been very casual about breaking the build on develop, perhaps worth discussing this as this PR should be fully green (as function of my changes)

PrivateKey: privateKey,
WalletType: model.UserWallet,
}
} else if isTrackOnly {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gocritic suggestion declined to keep changeset minimal and targeted, since other work is being done on other wallet types (SP wallets)

ContactInfo string `json:"contactInfo"`
Location string `json:"location"`
PrivateKey string `json:"privateKey,omitempty" table:"-"`
WalletType WalletType `gorm:"default:'UserWallet'" json:"walletType"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gocritic suggestion delcined since this follows style pattern of the migrations and commits introducing that were merged to develop despite raised checks


// extractClientFromRawJSON quickly extracts the client ID from raw JSON without full parsing
func (p *BatchParser) extractClientFromRawJSON(data []byte) (string, bool) {
// Use gjson for fast client extraction from raw JSON
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

might actually need to fix this since the handling was restructured


// shouldProcessDeal quickly checks if a deal should be processed based on client ID
func (p *BatchParser) shouldProcessDeal(data []byte) bool {
// Fast client check using gjson
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't even know this was a rule and my local checks did not flag, ok can do

}

// parseDeal parses a full deal from raw JSON data
func (p *BatchParser) parseDeal(data []byte) (*ParsedDeal, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ibid

})
}

func TestDealStateStreamFromHttpRequest_Compressed(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

replaced by the batch parser test

@Sankara-Jefferson Sankara-Jefferson self-requested a review July 16, 2025 00:46
@Sankara-Jefferson
Copy link
Contributor

Sankara-Jefferson commented Jul 18, 2025

@parkan
Unit and E2E Tests

  • The only test files found and run were for dealtracker logic. No unit or E2E test files for the new wallet tracking CLI (cmd/wallet/track.go) or handler (handler/wallet/track_e2e_test.go) were found. Ensure comprehensive unit and E2E tests exist for wallet tracking commands and handlers.
  • Documentation: Add or update main docs and API references for wallet tracking, including usage examples and options.

@Sankara-Jefferson
Copy link
Contributor

@parkan I think the track option you just added should also support the additional metadata (name, contact, location. etc.) FFWD could use it to track all deal states of their grantees' deals on-chain.

@Sankara-Jefferson
Copy link
Contributor

@parkan, It looks like your branch was created from develop on July 11, 2025, after the deal template integration was merged. However, there have been additional changes merged into develop since then that are not included in your PR.

Before merging, please:

  • Rebase your branch onto the latest develop to ensure all recent changes are included and avoid any missing features or regressions.
  • Resolve any conflicts and verify that all critical features (such as wallet balance) remain intact after the rebase.
  • Once this is done, I’ll proceed with the merge. Let me know if you have any questions or need help with the rebase process.

@parkan
Copy link
Collaborator Author

parkan commented Jul 22, 2025

ok sure I can rebase migrations again (see my above comment) etc as well as add support for contact metadata and such, easy

@parkan
Copy link
Collaborator Author

parkan commented Jul 22, 2025

also I forcefully disagree with this:

The only test files found and run were for dealtracker logic. No unit or E2E test files for the new wallet tracking CLI (cmd/wallet/track.go) or handler (handler/wallet/track_e2e_test.go) were found. Ensure comprehensive unit and E2E tests exist for wallet tracking commands and handlers.

the e2e tests are highly comprehensive and equal or better to anything in the codebase

@Sankara-Jefferson
Copy link
Contributor

I will take a look at them once its rebased. It very possible I missed them due to the corruption.

parkan added 3 commits July 23, 2025 19:58
- Replace jstream with custom streaming parser using gjson
- Add fast-path client filtering (50x+ faster for non-matching deals)
- Implement parallel worker architecture (4 workers) for deal processing
- Add batch processing to reduce database contention
- Stream 40GB dataset in 4MB chunks to avoid memory issues
- Achieve 3-4x throughput improvement with reduced context switches

Changes:
- Custom ParsedDeal type and BatchParser for efficient processing
- Memory-efficient streaming with gjson (allows efficient single field extraction)  for JSON parsing
- Worker goroutines for parallel deal processing
- Configurable batch sizes (default 100 deals/batch)
- Better CPU utilization through reduced map[string]interface{} overhead"
…ate keys

Implements TrackedWallet functionality to enable deal tracking for arbitrary
client wallets without requiring private key import, enhancing security for
deal monitoring use cases.

```bash
$ singularity wallet track f01131298
$ singularity run deal-tracker [...]
```

Deals will be tracked as if they were made by an owned wallet.

**Model & Migration:**
- Add TrackedWallet to WalletType enum alongside UserWallet and SPWallet
- Create database migration (202507091100) for TrackedWallet support
- Resolve ActorID to address via Lotus API to satisfy unique constraints

**CLI Interface:**
- Add `singularity wallet track <actor_id>` command
- Integrates with existing wallet management workflow
- Uses default Lotus API endpoint with graceful fallback

**Implementation Details:**
- TrackedWallet stores ActorID for deal matching (deal tracker unchanged)
- Resolves ActorID to address using Filecoin.StateAccountKey API call
- No private key storage for TrackedWallet type
- Validates ActorID format (must start with f0/t0)

**Error Handling & UX:**
- User-friendly error messages for common mistakes:
  - Storage provider ActorIDs: "is a storage provider, not a client wallet"
  - Non-existent ActorIDs: "does not exist on the network"
  - Invalid actor types: "is not a client wallet"
- Graceful handling of Lotus API connectivity issues

**Testing:**
- Comprehensive unit tests with mocked Lotus client
- E2E tests with real Lotus API calls (skipped if network unavailable)
- Error scenario validation for all edge cases
- Tests multiple TrackedWallet creation and unique constraint handling

Enables minimal deal-tracker deployments for monitoring client wallets without
exposing private keys, e.g.:
- Third-party deal monitoring services
- Read-only tracking of client deal activity
- Insecure deployments which may leak private keys

Fully backward compatible - existing UserWallet and SPWallet functionality
unchanged. Deal tracker continues matching on ActorID as before. Attempting
to sign or send messages from a tracked-only wallet will fail in the same
way as with an SPWallet.

- Removing a wallet will not remove tracked deals (but stop updating them).
  This is as per existing implementation and needs PM input to address.
- Error when trying to sign messages with a non-user wallet is not helpful.
  (ibid)
@parkan parkan force-pushed the feat/standalone-dealtracker branch from 697782c to 940d3e8 Compare July 23, 2025 18:00
@parkan
Copy link
Collaborator Author

parkan commented Jul 23, 2025

ok, the conflicts should be resolved now

the longer the develop branch languishes in this state the more difficult the conflict resolution will get, of course

Copy link
Contributor

@Sankara-Jefferson Sankara-Jefferson left a comment

Choose a reason for hiding this comment

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

This PR introduces tracked wallet support, allowing monitoring of wallets without private keys, and delivers major performance improvements to the deal tracker via streaming and batch processing. The implementation is robust, well-tested (unit and E2E), and thoroughly documented. Thanks for the valuable contribution—merging!

@Sankara-Jefferson Sankara-Jefferson merged commit 36d4df7 into develop Jul 23, 2025
7 of 13 checks passed
@Sankara-Jefferson Sankara-Jefferson deleted the feat/standalone-dealtracker branch July 23, 2025 18:55
parkan added a commit that referenced this pull request Oct 22, 2025
…ate keys (#550)

This PR introduces tracked wallet support, allowing monitoring of wallets without private keys, and delivers major performance improvements to the deal tracker via streaming and batch processing. The implementation is robust, well-tested (unit and E2E), and thoroughly documented. Thanks for the valuable contribution—merging!

The failing CIs will be fixed on separate PRs.
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.

3 participants