Skip to content

feat: AgentFolio + Beacon integration - Bounty #2890 (200 RTC) - #7875

Open
IcanBENCHurCAT wants to merge 2 commits into
Scottcjn:mainfrom
IcanBENCHurCAT:feat/issue2890-agentfolio-beacon-upstream
Open

feat: AgentFolio + Beacon integration - Bounty #2890 (200 RTC)#7875
IcanBENCHurCAT wants to merge 2 commits into
Scottcjn:mainfrom
IcanBENCHurCAT:feat/issue2890-agentfolio-beacon-upstream

Conversation

@IcanBENCHurCAT

Copy link
Copy Markdown
Contributor

Complete AgentFolio Beacon Integration reference implementation. 68 tests passing. 12 files, 2793 insertions.

@github-actions github-actions Bot added documentation Improvements or additions to documentation BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) size/XL PR: 500+ lines labels Jul 3, 2026

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary

This PR implements the AgentFolio ↔ Beacon integration (Bounty #2890, 200 RTC scope).

✅ What's Done Well

  1. Clean Architecture: The modular design with BeaconBridge, AgentFolio, and EnvelopeAttestation classes follows separation of concerns
  2. Comprehensive Tests: Test files cover folio assembly, bridge routing, and attestation verification
  3. Good Documentation: README.md includes architecture diagram, quick start, and usage examples
  4. Security: Ed25519 signatures for attestations with proper verification

🔍 Points to Consider

  1. Error Handling: Consider adding more specific exception types in bridge.py for better debugging
  2. Rate Limiting: The BeaconBridge might need rate limiting when hitting Beacon Atlas APIs in production
  3. Caching Strategy: Consider caching folio data with TTL to reduce API calls

📝 Minor Suggestions

  • Add type hints to all public methods
  • Consider adding integration tests with live Beacon Atlas endpoint
  • The .gitignore in the issue directory is good practice

Overall Assessment

APPROVE — The implementation is solid, well-tested, and documented. This is a high-quality submission for the MVP scope.


Reviewer: @jaxint (Hermes Agent)
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@lequangsang01

Copy link
Copy Markdown
Contributor

Technical Review: AgentFolio + Beacon Integration (PR #7875)

This is a well-structured integration that bridges Beacon Atlas and Agent Economy systems. I have a few technical observations:

1. Nonce Determinism in attestation.py (_generate_nonce)

The _generate_nonce function derives the nonce deterministically from blake2b(submission_id + timestamp). While this ensures uniqueness across different submissions, the same submission_id submitted at the exact same second would produce a duplicate nonce. In a race condition scenario (e.g., automated retries within the same Unix second), this could cause attestation collisions. Consider adding the agent_id to the hash input or using a random component to guarantee uniqueness.

2. Field Name Mapping Inconsistency in attestation.py (EnvelopeAttestation.from_envelope)

The from_envelope classmethod maps envelope["pubkey"] to self.pubkey_hex and envelope["sig"] to self.sig_hex. This asymmetric naming (pubkey vs pubkey_hex, sig vs sig_hex) could cause confusion for consumers who expect consistent field names between the envelope dict and the dataclass. The to_envelope method outputs "pubkey" and "sig" (transport names), but the dataclass attributes add _hex suffixes. A comment clarifying the naming convention would help future maintainers distinguish transport format from internal representation.

3. Private API Dependency in bridge.py (BeaconBridge._request)

BeaconBridge._request delegates to economy_client._request(method, endpoint, base_url=...). The base_url kwarg override is an internal SDK detail not guaranteed by any public API contract. The Caveats section documents this well, but adding a runtime check in the bridge constructor that the SDK supports the base_url override would provide early failure signals if the SDK changes its internal interface.

Overall solid work - the graceful degradation pattern and comprehensive test coverage (68 tests) are strong design choices.

@Scottcjn

Scottcjn commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Reviewed properly: this is real work, the Ed25519 envelope attestation, the Beacon bridge, and the test suite are substantive, and the priority window from the earlier claimant expired in April so the lane is open. Three things before payout. One: the code sits in Rustchain/issue-2890/, a repo-name-nested directory; its own README says bounties/issue-2890, so move it there. Two: repo CI does not collect your 68 tests, so wire them into the workflow or include a run log we can reproduce. Three: the bounty rate moved with the published reduction schedule, the issue title now reads 67 RTC, so that is the payout figure, not 200. Land those three and this pays.

@Scottcjn

Copy link
Copy Markdown
Owner

This is substantive, real work — ~835 LOC of Ed25519-signed Beacon v2 attestations with canonical-JSON signing and graceful crypto fallback, plus ~1195 LOC of tests and a spec/demo. Appreciated, and it's clearly not a farm. One blocker before merge: the files land in Rustchain/issue-2890/ (a nested Rustchain/ subdirectory), while your own README and report say bounties/issue-2890/. Please move them to bounties/issue-2890/ (or wherever the bounty spec directs) so it's in the right place, and double-check the AgentEconomyClient._request import against current SDK internals. Fix the placement and we'll merge + pay #2890.

@Scottcjn

Copy link
Copy Markdown
Owner

Both you and beacon-skill #903 did substantial work on #2890, so we cherry-picked from both and split the 200 RTC by contribution. You get 90 RTC (45%) — the complete AgentFolio-Beacon reference implementation, 68 tests, and full docs/SPEC are the broadest, most complete package. The 55/45 lean went to #903 only because it carried the cryptographically-proven cross-linkage that's the security core of this specific bounty. Genuinely solid, honest work here (clean author attribution, real tests). tx e7e38a0b26346a3eb9a81c825bd81507, pid 3753, ~24h. — Sophia, Elyan Labs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) documentation Improvements or additions to documentation size/XL PR: 500+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants