feat: Add BAP-578 NFA Development skill#2
feat: Add BAP-578 NFA Development skill#2saiboyizhan wants to merge 3 commits intoChatAndBuild:mainfrom
Conversation
christelbuchanan
requested review from
christelbuchanan,
highsoft-85,
pivotalzhang and
snowmanxm
|
Thanks @saiboyizhan we will review |
|
@saiboyizhan some ideas. This is a nice structure. The biggest improvements are about (1) separating what BAP-578 requires vs what the reference contract chooses, and (2) aligning interface/state/lifecycle with the actual spec ( Below are the concrete fixes I’d make (with suggested drop-in wording/snippets). 1) Correctness + spec alignment fixes (high priority)A) Don’t claim UUPS / Enumerable / URIStorage are “BAP-578 inherits from”BAP-578 defines an interface and behaviors (state/status/metadata/learning modules/vault permissions/circuit breakers), but it does not mandate UUPS or specific OpenZeppelin mixins. Your current “BAP-578 inherits from … UUPSUpgradeable” reads like the standard requires it, which is not true. ([GitHub]1) Replace with:
B) Update Agent State to match BAP-578 core interfaceSpec Your Suggested struct (spec-accurate): enum Status { Active, Paused, Terminated }
struct AgentState {
uint256 balance;
Status status;
address owner;
address logicAddress;
uint256 lastActionTimestamp;
// (optional extension) uint256 createdAt;
}Call out C) Add the missing core function:
|
- Separate spec requirements vs reference implementation choices - Add AgentState with Status enum (Active/Paused/Terminated) - Add executeAction as core operation - Add Learning Module (dual-path: JSON Light Memory + Merkle Tree) - Add Vault Permission System and Memory Module Registry - Add three-level circuit breaker security - Mark withdrawFromAgent as reference implementation feature - Update frontmatter with tags, requires, and new examples - Update deployment section with env vars and RPC docs link
|
Thank you Christel! Glad the contributions are helpful — BAP-578 is a great standard to build on. |
|
Thank you for the thorough review! I've addressed all 6 categories of feedback:
Also updated frontmatter (tags, requires, examples) and deployment section (env vars, RPC docs link). Please let me know if anything needs further adjustment! |
|
wow super fast thank you @saiboyizhan , @snowmanxm will double check now |
| ### Required Events | ||
|
|
||
| ```solidity | ||
| event ActionExecuted(uint256 indexed tokenId, bytes data); | ||
| event LogicUpgraded(uint256 indexed tokenId, address indexed newLogicAddress); | ||
| event AgentFunded(uint256 indexed tokenId, uint256 amount); | ||
| event StatusChanged(uint256 indexed tokenId, Status newStatus); | ||
| event MetadataUpdated(uint256 indexed tokenId); |
There was a problem hiding this comment.
“Required Events” section does not match BAP-578 event signatures
“Required Events” differ from the spec interface (parameter types/fields are different, and one required field is omitted). This can cause developers to implement incompatible events while believing they are spec-compliant.
There was a problem hiding this comment.
Fixed in 43144ec — aligned all event signatures with the actual BAP-578 spec:
- Replaced
ActionExecuted,LogicUpgraded,StatusChangedwith correct spec events (AgentCreated,AgentWithdraw,AgentStatusChanged,LogicAddressUpdated) - Updated all event references in operation descriptions to match
- Added a note clarifying that
ActionExecutedis implementation-specific (e.g.,NFAPredictionAgent), not defined in the core spec
Thanks for catching this @snowmanxm!
- Replace ActionExecuted/LogicUpgraded/StatusChanged with actual spec events - Add missing AgentCreated, AgentWithdraw events - Fix event name references in operation descriptions - Clarify that ActionExecuted is implementation-specific, not in core spec Addresses review feedback from @snowmanxm
clean up: reorganize blockchain skills
Greptile SummaryThis PR introduces a new skill document for the BAP-578 Non-Fungible Agent (NFA) standard on BNB Chain. The skill is comprehensive, covering the contract architecture, core interface, lifecycle operations, optional learning and vault modules, security patterns, and deployment guidance. It is a valuable addition to the skills store and fills a genuine gap for ChatAndBuild's own token standard. However, three technical issues in the documented code samples need to be addressed before merging:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Owner
participant NFA as NFA Contract
participant Logic as LogicAddress Contract
participant LM as Learning Module
participant Vault as Vault Permission Manager
Owner->>NFA: createAgent(to, logicAddress, metadataURI, metadata)
NFA-->>Owner: tokenId
Owner->>NFA: fundAgent(tokenId) [payable]
NFA-->>NFA: AgentState.balance += msg.value
Owner->>NFA: executeAction(tokenId, data)
NFA->>NFA: require(status == Active)
NFA->>Logic: delegatecall execute(tokenId, data)
Note over Logic,NFA: Runs in NFA contract storage context
Logic-->>NFA: result
NFA-->>NFA: lastActionTimestamp = block.timestamp
Owner->>NFA: pause(tokenId)
NFA-->>NFA: status = Paused
Owner->>NFA: unpause(tokenId)
NFA-->>NFA: status = Active
Owner->>NFA: setLogicAddress(tokenId, newLogic)
NFA-->>NFA: logicAddress = newLogic
Owner->>NFA: terminate(tokenId)
NFA-->>Owner: return remaining balance
NFA-->>NFA: status = Terminated (permanent)
Owner->>LM: updateLearning(tokenId, LearningUpdate)
LM-->>NFA: learningTreeRoot updated (32 bytes on-chain)
Owner->>Vault: delegateAccess(tokenId, delegate, level, expiry, sig)
Vault-->>Vault: Permission stored (time-bound)
Last reviewed commit: "fix: align Required ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| struct LearningUpdate { | ||
| bytes32 previousRoot; // Prior Merkle root | ||
| bytes32 newRoot; // Updated Merkle root | ||
| bytes32 proof; // Merkle proof for verification | ||
| bytes32 metadata; // Associated update metadata | ||
| } | ||
|
|
||
| struct LearningMetrics { | ||
| uint256 totalInteractions; // Count of agent interactions | ||
| uint256 learningEvents; // Significant learning milestones | ||
| uint256 lastUpdateTimestamp; // Most recent update time | ||
| uint256 learningVelocity; // Rate of learning (scaled 1e18) | ||
| uint256 confidenceScore; // Agent confidence (scaled 1e18) | ||
| } | ||
| ``` | ||
|
|
||
| ### ILearningModule Interface | ||
|
|
||
| ```solidity | ||
| function updateLearning(uint256 tokenId, LearningUpdate calldata update) external; | ||
| function verifyLearning(uint256 tokenId, bytes32 claim, bytes32[] calldata proof) external view returns (bool); |
There was a problem hiding this comment.
LearningUpdate.proof type mismatch with verifyLearning
The proof field in LearningUpdate (line 231) is typed as bytes32 (a single hash), but verifyLearning in ILearningModule (line 248) accepts bytes32[] calldata proof — an array of hashes. These two definitions are inconsistent.
A Merkle proof must contain one sibling hash per level of the tree. A single bytes32 value is only sufficient for a tree of height 1 (i.e., exactly 2 leaves). For any practical learning tree, the proof must be an array.
Developers implementing updateLearning will need to pass a LearningUpdate struct — but the bytes32 proof field cannot hold the multi-element proof array that verifyLearning expects. This will either force implementations to ignore the struct's proof field or to break the interface contract.
The struct should be corrected to:
| struct LearningUpdate { | |
| bytes32 previousRoot; // Prior Merkle root | |
| bytes32 newRoot; // Updated Merkle root | |
| bytes32 proof; // Merkle proof for verification | |
| bytes32 metadata; // Associated update metadata | |
| } | |
| struct LearningMetrics { | |
| uint256 totalInteractions; // Count of agent interactions | |
| uint256 learningEvents; // Significant learning milestones | |
| uint256 lastUpdateTimestamp; // Most recent update time | |
| uint256 learningVelocity; // Rate of learning (scaled 1e18) | |
| uint256 confidenceScore; // Agent confidence (scaled 1e18) | |
| } | |
| ``` | |
| ### ILearningModule Interface | |
| ```solidity | |
| function updateLearning(uint256 tokenId, LearningUpdate calldata update) external; | |
| function verifyLearning(uint256 tokenId, bytes32 claim, bytes32[] calldata proof) external view returns (bool); | |
| struct LearningUpdate { | |
| bytes32 previousRoot; // Prior Merkle root | |
| bytes32 newRoot; // Updated Merkle root | |
| bytes32[] proof; // Merkle proof for verification (array of sibling hashes) | |
| bytes32 metadata; // Associated update metadata | |
| } |
| event AgentCreated(uint256 indexed tokenId, address indexed owner, address logicAddress, string metadataURI); | ||
| event AgentFunded(uint256 indexed tokenId, uint256 amount); | ||
| event AgentWithdraw(uint256 indexed tokenId, uint256 amount); | ||
| event AgentStatusChanged(uint256 indexed tokenId, bool active); |
There was a problem hiding this comment.
AgentStatusChanged event cannot represent all three lifecycle states
The spec defines a three-state lifecycle (Active, Paused, Terminated), but the AgentStatusChanged event uses a bool active. A boolean can only capture two states — both Paused and Terminated would emit false, making it impossible for event listeners, indexers, or UIs to distinguish between a temporary pause and a permanent termination.
This is particularly problematic because terminate() is documented as "permanent — cannot be reactivated, remaining balance is returned to the owner." These are fundamentally different semantics from pause(), yet the event would look identical to observers.
Consider updating the event signature to use the Status enum directly:
| event AgentStatusChanged(uint256 indexed tokenId, bool active); | |
| event AgentStatusChanged(uint256 indexed tokenId, Status newStatus); |
| - Agent must be in `Status.Active` — reverts if Paused or Terminated | ||
| - Delegates execution to the contract at `logicAddress` | ||
| - Updates `lastActionTimestamp` after execution | ||
| - **Gas safety:** implementations should cap gas for delegatecall (spec suggests `MAX_GAS_FOR_DELEGATECALL = 3_000_000`) |
There was a problem hiding this comment.
delegatecall storage context warning for executeAction
The documentation states that executeAction "delegates execution to the contract at logicAddress" and caps gas, but it omits a critical security implication: delegatecall runs the logic contract's code in the context of the NFA contract's storage. This means a malicious or buggy logic contract has unconstrained read/write access to all of the NFA contract's state, not just the individual agent's data.
Concretely:
- A malicious
logicAddresscan overwriteAgentStatefor every token in the contract, not just the one being executed. - It can zero out
AgentState.balancefor all agents and drain the entire contract's BNB viaselfdestruct(pre-Dencun), storage manipulation followed by a withdrawal, or by altering ownership mappings. - In UUPS contracts it can overwrite the implementation slot, permanently bricking the proxy.
This is arguably the most dangerous attack surface in the entire standard. It should be prominently documented — both in the "Execute Action" section and in "Security." Developers should understand that logicAddress must be treated with the same trust level as a contract admin.
Consider adding an explicit security callout here:
> **SECURITY — delegatecall runs in NFA contract storage context.** The logic contract at `logicAddress` has unconstrained access to all storage slots in the NFA contract. Only point `logicAddress` to contracts you fully audit and trust. Malicious or buggy logic contracts can drain all agent balances, corrupt ownership, or brick a UUPS proxy.
3 participants
Summary
Adds the first BAP-578 Non-Fungible Agent (NFA) development skill to the ChatChat Skills Store.
BAP-578 is ChatAndBuild's own token standard — the first BNB Chain Application Proposal for on-chain AI agents — but there was no corresponding skill in the store until now.
What this skill covers
Author context
I've been actively contributing to the BAP-578 standard: