fixed NatSpec in AquaApp

[Sunnesoft]

No description provided.

[claude]

Claude finished @Sunnesoft's task —— View job

---

PR Review: Fixed NatSpec in AquaApp

Summary

This PR updates the NatSpec documentation in AquaApp.sol to correctly reflect the actual function signature of the nonReentrantStrategy modifier, which requires both maker and strategyHash parameters. The corresponding documentation in README.md is also updated to maintain consistency.

Changes Reviewed

1. src/AquaApp.sol - NatSpec Documentation Fix

The NatSpec comment for using _safeCheckAquaPush() was updated to correctly show:

• ✅ Correct: modifier nonReentrantStrategy(maker, strategyHash)
• ✅ Correct: _reentrancyLocks[maker][strategyHash].lock()
• ❌ Removed: modifier nonReentrantLock(_reentrancyLocks[strategyHash]) (which was incorrect)

2. README.md - Documentation Updates

• Updated the example code snippet to show the correct modifier usage with both parameters
• Updated the API reference section to reflect the two-level mapping structure for reentrancy locks
• Fixed the modifier signature in the documentation

Code Quality Assessment

✅ Good Practices:

• Documentation accurately reflects the actual code implementation
• Maintains consistency between code comments and external documentation
• The fix addresses what appears to be an inconsistency that could confuse developers

Potential Issues

⚠️ Minor Concern:

• The PR description is empty. It would be helpful to include context about this being an OpenZeppelin audit finding (based on the branch name) for better traceability.

Security Considerations

✅ No security issues identified:

• This is purely a documentation fix with no code logic changes
• The actual reentrancy protection mechanism remains unchanged
• The two-parameter approach (maker + strategyHash) is the correct design to prevent cross-maker reentrancy attacks

Test Coverage

✅ No impact on test coverage:

• Since this is only a documentation change, existing tests remain valid
• The examples/apps/XYCSwap.sol already uses the correct signature

Recommendations

1. Add PR description: Consider adding a brief description mentioning this addresses the OpenZeppelin audit finding about inconsistent modifier documentation.

2. No code changes needed: The documentation fixes are accurate and complete.

Conclusion

This PR correctly fixes the documentation inconsistency in NatSpec comments and README. The changes ensure developers have accurate information when implementing AquaApp-based contracts. No functional changes were made, making this a safe documentation-only update.

Recommendation: Approve ✅

[Copilot]

Pull request overview

This PR updates the NatSpec documentation and code examples in AquaApp to reflect changes in the reentrancy protection mechanism. The reentrancy lock structure has been changed from a single-level mapping by strategy hash to a nested mapping by maker address and strategy hash.

Changes:

• Updated NatSpec comments to reflect new nonReentrantStrategy modifier signature
• Updated code examples in README to use new two-parameter modifier signature
• Updated mapping structure documentation to reflect nested maker/strategy hash mapping

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/AquaApp.sol	Updated NatSpec documentation for reentrancy protection to reflect new modifier signature and lock structure
README.md	Updated code examples and documentation to match new reentrancy protection API with maker parameter

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.