feat: payment processor #5
Conversation
update `feat/payment-processor` branch
Co-Authored-By: hey-ewan <[email protected]> Co-Authored-By: g4titanx <[email protected]>
WalkthroughThis pull request introduces several updates across the repository. Two new environment variables have been added to the environment example file. Dependency versions have been bumped in the package configuration. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FR as ForgeRegistry
participant LM as LicensingModule
U->>FR: Initialise contract (ipAssetRegistry, workflows, licensing, etc.)
Note right of FR: Sets licensing, royalty, and batcher configurations
U->>FR: Call register(receiver, IPMetadata, appId, submitter)
FR->>FR: Validate parameters and update state
FR->>LM: Attach licensing terms
FR-->>U: Emit events (e.g. PaymentReceived, FundsWithdrawn)
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
execution/contracts/src/ForgeRegistry.sol (3)
16-17: Remove duplicate import.Line 16 and line 17 import the same contract
RoyaltyPolicyLAP. Please remove one of the imports to avoid confusion.- import {RoyaltyPolicyLAP} from "@storyprotocol/core/modules/royalty/policies/LAP/RoyaltyPolicyLAP.sol";
122-126: Minimise repeated ownership initialisation.You are calling
__Ownable_init(owner)and_transferOwnership(owner)in the same function. Typically,__Ownable_init(owner)is sufficient to set up ownership. Re-check if the second call to_transferOwnership(owner)is needed.
281-299: Emitted eventBalanceLockedis confusing duringwithdraw.You emit
BalanceLocked(msg.sender)on line 296 within thewithdraw()function after resettingunlockBlockTime. This may cause confusion, as the user is withdrawing rather than explicitly re-locking funds. Consider using a more specific event name or clarifying the rationale.execution/crates/payment/src/lib.rs (2)
1-5: These public functions currently have no implementation.Consider adding logic or documentation to detail how
deposit_to_forge,check_balance_on_forge, andestimate_feeshould operate. This helps consumers understand the intended usage.
7-10: Basic test module declared.At present, the test module is empty. Add unit tests for these new public functions to ensure correctness once they are implemented.
execution/contracts/package.json (1)
2-8: Updated Dependencies Versions VerificationThe dependency versions for
@openzeppelin/contracts,@openzeppelin/contracts-upgradeable,@story-protocol/protocol-core, and@story-protocol/protocol-peripheryhave been bumped. Please ensure that these new versions are fully compatible with our contract implementations and that no breaking changes have been introduced by these updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
execution/Cargo.lockis excluded by!**/*.lockexecution/contracts/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.env.example(1 hunks)execution/contracts/package.json(1 hunks)execution/contracts/src/ForgeRegistry.sol(5 hunks)execution/contracts/src/ForgeStorage.sol(1 hunks)execution/crates/payment/Cargo.toml(1 hunks)execution/crates/payment/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .env.example
- execution/crates/payment/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (10)
execution/contracts/src/ForgeRegistry.sol (7)
24-24: Solid usage ofReentrancyGuardUpgradeable.Protecting your contract from re-entrancy attacks in payment and withdrawal operations is prudent.
97-102: Ensure correct initialisation with_batcherWallet.Including
_batcherWalletin the constructor ensures correct usage of theonlyBatchermodifier. Double-check that your deployment script passes the correct wallet address during initialisation.
219-229: Licensing terms integration appears cohesive.Registering the licence terms and attaching them to the IP is logically consistent with your current architecture. Ensure any future updates to
PILicenseTemplateorILicensingModuleremain backward compatible.
255-260: Thereceive()function is straightforward.Funds are credited to the sender’s balance, with the
unlockBlockTimereset. This aligns well with your deposit approach.
262-271: Possible edge case for zero-balance users.Calling
unlock()with a zero balance reverts, which is logical. Confirm there are no scenarios where a user might need to set an unlock time prior to depositing funds.
273-279: Lock function with zero-balance revert.Your logic prevents users from locking zero-balance accounts. While consistent, verify that no workflow requires a user to re-lock after fully withdrawing.
301-312: View functions are neatly structured.Accessors for balances, nonces, and unlock block times are clear and consistent with the
userDatamapping.execution/contracts/src/ForgeStorage.sol (2)
5-9: UserInfo struct covers essential fields.Storing
balance,unlockBlockTime, andnoncecaters to fundamental user fund management. Ensure future additions do not break storage layout.
11-15: Public mapping and batcherWallet variable are clearly defined.
batcherWalletfor theonlyBatcherchecks and theuserDatamapping for user funds tracking are both straightforward. The__GAParray is a sound approach for preserving storage space.execution/contracts/package.json (1)
9-12: DevDependencies ValidationThe
devDependenciesremain unaltered, which is acceptable as there was no need for version changes here. However, please verify that the existing versions ofds-testandforge-stdcontinue to support our testing framework without any issues.
| uint256 gasUsed = startGas - gasleft(); | ||
| uint256 refundAmount = gasUsed * tx.gasprice; | ||
|
|
||
| if (userData[submitter].balance < refundAmount) { | ||
| revert SubmissionInsufficientBalance( | ||
| submitter, | ||
| userData[submitter].balance, | ||
| refundAmount | ||
| ); | ||
| } | ||
|
|
||
| userData[submitter].nonce++; | ||
| userData[submitter].balance -= refundAmount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential risk of insufficient contract balance for refund.
The refund logic calculates the gas used and transfers the refundAmount to the batcher. Consider verifying the contract’s Ether balance before calling .transfer(), to prevent unintended reverts if the contract’s balance is insufficient, even if userData[submitter].balance covers the cost.
| payable(msg.sender).transfer(refundAmount); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider verifying contract balance before transferring.
If the contract’s Ether balance is below refundAmount, this call will revert. Checking address(this).balance >= refundAmount can provide a clearer error message.
🏁 Script executed:
#!/bin/bash
rg '\.transfer\(' -A 3Length of output: 558
Review: Add Balance Verification Before Fund Transfers
The refund transfer operation at
execution/contracts/src/ForgeRegistry.sol, lines 252‑253, currently executes
payable(msg.sender).transfer(refundAmount);without checking if the contract holds enough Ether to cover refundAmount. This can lead to an unexpected revert if the balance is insufficient. I recommend adding a balance check such as:
require(address(this).balance >= refundAmount, "Insufficient funds for refund");This approach not only prevents unintended transaction failures but also provides a clearer error message. Please consider applying a similar balance verification to other transfer operations in the contract if applicable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
execution/contracts/src/ForgeRegistry.sol (7)
2-2: Consider using a more flexible pragma version.The current pragma
^0.8.26is very specific. Consider using a more flexible version range like^0.8.0to ease future upgrades while maintaining compatibility with OpenZeppelin contracts.-pragma solidity ^0.8.26; +pragma solidity ^0.8.0;
27-27: Consider making UNLOCK_BLOCK_TIME configurable.The unlock time is hardcoded to 1 hour. Consider making it configurable by the contract owner to allow for adjustments based on business needs.
- uint256 public constant UNLOCK_BLOCK_TIME = 3600 seconds; + uint256 public UNLOCK_BLOCK_TIME;
45-49: Add NatSpec documentation for the Terms struct.The Terms struct lacks documentation explaining the purpose of each field. Consider adding NatSpec comments to improve code maintainability.
+ /// @notice Struct defining the terms of an IP license + /// @param transferable Whether the license can be transferred + /// @param commercial Whether commercial use is allowed + /// @param commercialAttribution Whether attribution is required for commercial use struct Terms { bool transferable; bool commercial; bool commercialAttribution; }
52-63: Consider adding more indexed parameters to events.The PaymentReceived and FundsWithdrawn events could benefit from indexing the amount parameter to allow efficient filtering of high-value transactions.
- event PaymentReceived(address indexed sender, uint256 amount); + event PaymentReceived(address indexed sender, uint256 indexed amount); - event FundsWithdrawn(address indexed recipient, uint256 amount); + event FundsWithdrawn(address indexed recipient, uint256 indexed amount);
93-135: Consider emitting an event after initialization.The initialize function performs critical setup but doesn't emit an event. Consider adding an event to log the initialization parameters for better transparency and auditability.
+ event ForgeRegistryInitialized( + address indexed ipAssetRegistry, + address indexed registrationWorkflows, + address indexed batcherWallet + ); function initialize( // ... parameters ... ) public initializer { // ... existing code ... + emit ForgeRegistryInitialized( + ipAssetRegistryAddress, + registrationWorkflowsAddress, + _batcherWallet + ); }
229-229: Consider making the gas refund multiplier configurable.The gas refund multiplier is hardcoded to 110%. Consider making it configurable by the contract owner to adjust for varying network conditions.
+ uint256 public gasRefundMultiplier = 110; // ... in register function ... - uint256 refundAmount = (gasUsed * tx.gasprice * 110) / 100; + uint256 refundAmount = (gasUsed * tx.gasprice * gasRefundMultiplier) / 100;
259-263: Consider adding a minimum deposit requirement.The receive function accepts any amount. Consider adding a minimum deposit requirement to prevent dust transactions that could lead to higher gas costs relative to the deposit amount.
+ uint256 public minDeposit = 0.01 ether; receive() external payable nonReentrant { + require(msg.value >= minDeposit, "Deposit amount too low"); userData[msg.sender].balance += msg.value; userData[msg.sender].unlockBlockTime = 0; emit PaymentReceived(msg.sender, msg.value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
execution/contracts/src/ForgeRegistry.sol(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (2)
execution/contracts/src/ForgeRegistry.sol (2)
80-85: LGTM! Well-implemented modifier with custom error.The onlyBatcher modifier effectively restricts access and provides clear error messages.
304-314: LGTM! Well-implemented view functions.The view functions provide clear access to user data without any state modifications.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
execution/contracts/src/ForgeRegistry.sol (4)
19-28: Constant naming consistency.
AlthoughUNLOCK_BLOCK_TIMEclearly states its purpose, you might consider adding further context in a comment to clarify that it equates to one hour, given its value of 3600 seconds. This can aid readability.
45-50: Unused struct detected.
TheTermsstruct currently has no usage in the contract. If you plan to incorporate customisable terms in a future iteration, consider adding a TODO note referencing the relevant ticket or roadmap item. Otherwise, it may be removed to minimise confusion.
66-68: Remove or implementNoSubmittererror.
TheNoSubmittererror is declared but never used in the contract. If you plan to throw this error under specific conditions, please implement it; otherwise, removal is recommended to keep the codebase clean.
183-257: Refund calculation and transfer considerations.
- The partial refund logic calculates
(gasUsed * tx.gasprice * 110) / 100. While this approach is valid, it is approximate and can be influenced by any gas overhead before measuringstartGas. Documenting the 110% premium or making it adjustable might help manage future fee changes.- Using
.transfer()enforces a 2300-gas stipend. This is typically safer but can fail ifmsg.senderis a contract with a complex fallback. If you anticipate calls from contracts that require more gas in fallback, consider using.callwithnonReentrant.- The checks for both contract balance and
userData[submitter].balancelook correct, preventing unintended reverts and freeload transactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
execution/contracts/src/ForgeRegistry.sol(5 hunks)execution/crates/payment/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/crates/payment/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
execution/contracts/src/ForgeRegistry.sol (4)
7-17: Imports look well-structured and cohesive.
No issues found regarding the addition ofReentrancyGuardUpgradeableand new Story Protocol imports. These imports seem necessary and appropriate for the new licensing and payment logic.
259-264: Safe deposit mechanism.
Using areceive()function withnonReentranteffectively prevents re-entrancy attacks when users send Ether. The immediate assignment touserData[msg.sender].balanceappears logically sound. Consider logging zero-value deposits if they are pertinent to your business logic.
266-283: Lock/unlock workflow is straightforward.
The approach to setunlockBlockTimeonunlock()and reset it to0onlock()is intuitively clear. If users callunlock()repeatedly, they repeatedly push back the unlock time. Ensure this is the desired behaviour for your user experience.
285-302: Withdrawal flow is aligned with user balance checks.
- Verifying
amountagainstsenderData.balanceand ensuring theunlockBlockTimehas passed are strong controls.- Resetting
unlockBlockTimeto0post-withdrawal encourages a fresh lock period before the next withdrawal.- Generally well-structured logic for fund release in an upgradable contract.
this is the initial version of our payment system designed to add cost coverage of transactions to the api
Summary by CodeRabbit
New Features
Chores