Skip to content

feat: add approve commands (create-claim, nft, erc20)#8

Merged
bengobeil merged 9 commits intomainfrom
gerald/dev-2391-approve-functions-cli
Mar 16, 2026
Merged

feat: add approve commands (create-claim, nft, erc20)#8
bengobeil merged 9 commits intomainfrom
gerald/dev-2391-approve-functions-cli

Conversation

@GeraldBennyClawBot
Copy link
Copy Markdown
Contributor

Implements DEV-2391. Adds bulla approve subcommand with three operations: create-claim (BullaApprovalRegistry.approveCreateClaim), nft (BullaClaimV2.approve ERC721), erc20 (ERC20.approve). Each has build and execute variants. Adds ABIs, domain types, encoder port/adapter, service layer, CLI options/validation/commands, and registry support for bullaApprovalRegistry + bullaClaimV2 addresses.

Add `bulla approve` subcommand with three operations:
- `create-claim` — BullaApprovalRegistry.approveCreateClaim
- `nft` — BullaClaimV2.approve (ERC721)
- `erc20` — ERC20.approve

Each has build (unsigned tx) and execute (sign+send) variants.

Adds ABIs, domain types, encoder port/adapter, service layer,
CLI options/validation/commands, and registry support for
bullaApprovalRegistry + bullaClaimV2 contract addresses.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Set 'approved' as default for --approval-type option
- Add explicit validateApprovalType() with InvalidApprovalTypeError
- Add unit tests for approve service (11 tests) and validation (16 tests)
- Add e2e tests for all 3 approve build commands + help output

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@bengobeil bengobeil left a comment

Choose a reason for hiding this comment

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

Still no support for approve transfers for BullaInvoice and BullaFrendLendV2. Write tests to confirm. Perhaps the whole e2e flow would be: mint invoice, approve transfer, then transfer, check that nft has changed ownership

Copy link
Copy Markdown
Contributor

@bengobeil bengobeil left a comment

Choose a reason for hiding this comment

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

Still no support for approve transfers for BullaInvoice and BullaFrendLendV2

Adds `bulla invoice approve-nft` and `bulla frendlend approve-nft`
subcommands. Removes standalone `bulla approve nft`. Both target
BullaClaimV2 via the registry since all claims are ERC721 on that
contract regardless of which controller created them.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@GeraldBennyClawBot
Copy link
Copy Markdown
Contributor Author

Pushed changes: approve-nft is now a subcommand on both invoice and frendlend (e.g. bulla invoice approve-nft build/execute, bulla frendlend approve-nft build/execute). Removed the standalone bulla approve nft command — bulla approve now only has create-claim and erc20. Added e2e tests for both new paths. All 172 unit tests + 8 e2e tests pass, typecheck clean.

…t commands

- buildApproveNft now targets the controller contract (BullaInvoice or
  BullaFrendLendV2) for controlled claims, not BullaClaimV2 directly
- Added transfer-nft subcommand to invoice and frendlend
- Added full lifecycle e2e tests: mint -> approve -> transfer -> verify
  ownership (per claim type, execute mode with anvil)
- Extended BullaClaimV2 ABI with transferFrom and ownerOf

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@GeraldBennyClawBot
Copy link
Copy Markdown
Contributor Author

Addressed all 3 review comments: (1) approve-nft and transfer-nft now target the controller contract (BullaInvoice or BullaFrendLendV2) instead of BullaClaimV2 directly. (2) Full lifecycle e2e tests using execute mode with anvil: invoice create->approve->transfer->check ownership, frendlend offer->accept->approve->transfer->check ownership. (3) Added invoice transfer-nft and frendlend transfer-nft subcommands. 178 unit + 4 e2e tests pass, 9 lifecycle tests skipped without anvil.

Uses publicnode.com Sepolia RPC as default when SEPOLIA_RPC_URL env
var is not set, so approve/transfer lifecycle tests always run.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@GeraldBennyClawBot
Copy link
Copy Markdown
Contributor Author

Round 3 review — all feedback addressed

1. approve-nft now targets the controller contract

  • buildApproveNft accepts a getContractAddress callback that determines where the approve tx is sent
  • Invoice approve-nft → targets BullaInvoice controller address (not BullaClaimV2)
  • FrendLend approve-nft → targets BullaFrendLendV2 controller address
  • Helper functions: getInvoiceControllerAddress, getFrendLendControllerAddress, getClaimContractAddress

2. transfer-nft commands added

  • bulla invoice transfer-nft build/execute — transfers invoice NFTs via BullaInvoice controller
  • bulla frendlend transfer-nft build/execute — transfers loan NFTs via BullaFrendLendV2 controller
  • Same getContractAddress callback pattern as approve-nft

3. Full lifecycle e2e tests (execute mode)

Invoice lifecycle (4 tests):

  1. Create invoice (mint) → get claimId
  2. invoice approve-nft execute → approve account1
  3. invoice transfer-nft execute → transfer NFT to account1
  4. Verify ownerOf(claimId) changed to account1

FrendLend lifecycle (5 tests):

  1. Offer loan → get offerId
  2. Accept loan → get claimId, verify initial owner
  3. frendlend approve-nft execute → approve account1
  4. frendlend transfer-nft execute → transfer NFT to account1
  5. Verify ownerOf(claimId) changed to account1

4. Public Sepolia RPC fallback

  • SEPOLIA_RPC_URL defaults to https://ethereum-sepolia-rpc.publicnode.com when env var is not set
  • Approve/transfer lifecycle tests no longer skip — they always run

CI passing ✅ (178 unit tests + 8 e2e build tests, typecheck clean)

- Create NftTransferService as Effect Context service with buildApproveNft
  and buildTransferNft methods
- Use Layer.effect factory (makeNftTransferServiceLayer) to create
  controller-specific service instances (Invoice, FrendLend, Claim)
- Provide NftTransferService via pipe in invoice/frendlend commands
- Switch from transferFrom to safeTransferFrom for NFT transfers
- Clean up approve-service.ts to only contain create-claim and ERC20

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
safeTransferFrom through the controller reverts with
ERC721InsufficientApproval when called by an approved (non-owner)
party on the deployed Sepolia contracts. transferFrom works correctly
for the same flow (approve → transfer by approved party).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@bengobeil bengobeil left a comment

Choose a reason for hiding this comment

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

use SafeTransferFrom

Gerald (AI Assistant) and others added 2 commits March 16, 2026 16:43
The 3-arg safeTransferFrom(address,address,uint256) is an external wrapper
that internally calls the 4-arg public version. By calling the 4-arg
safeTransferFrom(address,address,uint256,bytes) directly with empty bytes,
we avoid any msg.sender issues in the external-to-public delegation path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Hardhat's well-known test accounts (0xf39Fd6e..., 0x70997970...) have
EOF contracts deployed on Sepolia, causing safeTransferFrom to call
onERC721Received and revert. Clear account code via anvil_setCode
after fork starts so these addresses behave as EOAs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@bengobeil bengobeil merged commit 874350a into main Mar 16, 2026
1 check passed
@bengobeil
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 0.0.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants