Sponsor payment #326

Kolezhniuk · 2024-12-23T07:48:55Z

Some details:

Sponsor deposits coins/tokens
Sponsor creates eip-712 signature to allow the recipient to withdraw money
Recipient could withdraw money until expiration time passed
Sponsor can withdraw tokens/coins back but after withdrawal delay period
Recipient claims payment with fee that is specified by contract owner
Owner could transfer coins/tokens to his address

vmidyllic · 2024-12-23T09:51:10Z

contracts/payment/SponsorPayment.sol

+ *      Supports Ether and ERC-20 token payments with enhanced security feature$.
+ * @custom:storage-location erc7201:iden3.storage.SponsorPayment
+ */
+contract SponsorPayment is ReentrancyGuardUpgradeable, EIP712Upgradeable, Ownable2StepUpgradeable {


what is a motivation to use nonReentrant functionality in this contract?

to prevent nonReentrant attack

this is only protection from internal calls,no?

vmidyllic · 2024-12-23T09:52:39Z

contracts/payment/SponsorPayment.sol

+    /**
+     * @dev Payment details used in claim logic.
+     */
+    struct ERC20SponsorPaymentInfo {


honestly I don't like the word Sponsor in the context of prepaid verificactions. Open discussion @demonsh @Kolezhniuk @AndriianChestnykh

@vmidyllic What is your suggestion?

vmidyllic · 2024-12-23T09:53:52Z

contracts/payment/SponsorPayment.sol

+        if (withdrawalDelay == 0) revert InvalidParameter("Invalid withdrawal delay");
+        SponsorPaymentStorage storage $ = _getSponsorPaymentStorage();
+        $.ownerPercentage = ownerPercentage;
+        $.withdrawalDelay = withdrawalDelay;


is withdrawalDelay corresponds to all 'payments' or acts as a general configuration?

Didn't get your question yes it acts as a general configuration, so withdrawalDelay corresponds to all 'payments'

vmidyllic · 2024-12-23T10:02:15Z

contracts/payment/SponsorPayment.sol

+ *      Supports Ether and ERC-20 token payments with enhanced security feature$.
+ * @custom:storage-location erc7201:iden3.storage.SponsorPayment
+ */
+contract SponsorPayment is ReentrancyGuardUpgradeable, EIP712Upgradeable, Ownable2StepUpgradeable {


I want to ask @AndriianChestnykh if we inherit different upgradeable contracts and they are all Initializable ( have the same storage location), will be that a problem for upgrade?

No, there should be no problem for upgrades.

vmidyllic · 2024-12-23T10:03:46Z

contracts/payment/SponsorPayment.sol

+    struct SponsorPaymentStorage {
+        mapping(address => mapping(address => uint256)) balances; // sponsor => token => balance
+        mapping(address => mapping(address => WithdrawalRequest)) withdrawalRequests; // sponsor => token => request
+        mapping(bytes32 => bool) isWithdrawn;


it is not clear what is bytes32 without any comment

vmidyllic · 2024-12-23T10:13:46Z

contracts/payment/SponsorPayment.sol

+        $.withdrawalDelay = withdrawalDelay;
+        __ReentrancyGuard_init();
+        __EIP712_init("SponsorPayment", VERSION);
+        __Ownable_init(owner);


Should not it be __Ownable2Step_init ?

Not sure, maybe @AndriianChestnykh could help here

__Ownable2Step_init is an empty function. __Ownable_init looks correct for owner setup.

vmidyllic · 2024-12-23T10:14:57Z

contracts/payment/SponsorPayment.sol

+     * @param addr Address to check
+     */
+    function _isContract(address addr) private view returns (bool) {
+        uint256 size;


I guess it is the same check target.code.length > 0 (you can test)

replaced this check

vmidyllic · 2024-12-23T10:17:06Z

contracts/payment/SponsorPayment.sol

+        SponsorPaymentStorage storage $ = _getSponsorPaymentStorage();
+
+        if (msg.value == 0) revert InvalidDeposit("Invalid value amount");
+        $.balances[_msgSender()][address(0)] += msg.value;


do we deposit to 0 address for native token?

Yes, I'm store native coin deposit per specific sponsor under address(0)
Any objection or suggestions?

vmidyllic · 2024-12-23T10:19:23Z

contracts/payment/SponsorPayment.sol

+    function depositERC20(uint256 amount, address token) external nonReentrant validToken(token) {
+        SponsorPaymentStorage storage $ = _getSponsorPaymentStorage();
+        if (amount == 0) revert InvalidDeposit("Invalid token amount");
+        IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);


is allowance in token contract is given sepatelly?

You can see the interaction with SC by reviewing unit tests

vmidyllic · 2024-12-23T10:22:31Z

contracts/payment/SponsorPayment.sol

+        emit ERC20Withdrawal(_msgSender(), token, amount);
+    }
+
+    function _claimPayment(


this method name confuses me.

Please suggest the name that don't make you confuse

discussion in the separate thread. I'm against of the usage the word 'claim' to avoid missunderstanding.

coveralls · 2024-12-23T11:02:45Z

Pull Request Test Coverage Report for Build 12584161757

Details

108 of 118 (91.53%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.6%) to 83.412%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/payment/SponsorPayment.sol	108	118	91.53%

Totals
Change from base Build 12413543528:	-0.6%
Covered Lines:	1126
Relevant Lines:	1255

💛 - Coveralls

volodymyr-basiuk · 2024-12-23T14:21:36Z

contracts/payment/SponsorPayment.sol

+        0x98fc76e32452055302f77aa95cd08aa0cf22c02a3ebdaee3e1411f6c47c2ef00;
+
+    modifier validToken(address token) {
+        if (token != address(0)) {


so if token == address(0) - it is valid ?
based on the modifier

maybe
token == address(0) || token.code.length == 0 -> then revert

AndriianChestnykh · 2024-12-30T14:31:08Z

contracts/payment/SponsorPayment.sol

+ *      Supports Ether and ERC-20 token payments with enhanced security feature$.
+ * @custom:storage-location erc7201:iden3.storage.SponsorPayment
+ */
+contract SponsorPayment is ReentrancyGuardUpgradeable, EIP712Upgradeable, Ownable2StepUpgradeable {


No, there should be no problem for upgrades.

AndriianChestnykh · 2024-12-30T15:54:09Z

contracts/payment/SponsorPayment.sol

+    }
+
+    /**
+     * @dev Main storage $tructure for the contract


AndriianChestnykh · 2024-12-30T15:54:50Z

contracts/payment/SponsorPayment.sol

+     * @dev Main storage $tructure for the contract
+     */
+    struct SponsorPaymentStorage {
+        mapping(address => mapping(address => uint256)) balances; // sponsor => token => balance


Better use kind of "inline comments" for mappings mapping(address sponsor => mapping(address token => uint256 balance))

AndriianChestnykh · 2024-12-30T16:10:53Z

contracts/payment/SponsorPayment.sol

+        $.withdrawalDelay = withdrawalDelay;
+        __ReentrancyGuard_init();
+        __EIP712_init("SponsorPayment", VERSION);
+        __Ownable_init(owner);


__Ownable2Step_init is an empty function. __Ownable_init looks correct for owner setup.

AndriianChestnykh · 2024-12-31T13:05:54Z

contracts/payment/SponsorPayment.sol

+            "ERC20SponsorPaymentInfo(address recipient,uint256 amount,address token,uint256 expiration,uint256 nonce,bytes metadata)"
+        );
+
+    bytes32 public constant PAYMENT_CLAIM_DATA_TYPE_HASH =


Why not just SPONSOR_PAYMENT_INFO_TYPE_HASH for name consistency?

AndriianChestnykh · 2024-12-31T13:32:51Z

contracts/payment/SponsorPayment.sol

+        $.ownerPercentFee = ownerPercentFee;
+    }
+
+    function _tryRecoverSigner(


Please put all private and internal methods after public methods

AndriianChestnykh · 2024-12-31T13:53:18Z

contracts/payment/SponsorPayment.sol

+    struct WithdrawalRequest {
+        uint256 amount;
+        uint256 lockTime;
+        bool exists;


Can amount > 0 indicate that request exists to same 1 storage slot?

Yes, refactored

AndriianChestnykh · 2024-12-31T13:58:36Z

contracts/payment/SponsorPayment.sol

+
+    function _getWithdrawalAmount(address token) private view returns (uint256) {
+        SponsorPaymentStorage storage $ = _getSponsorPaymentStorage();
+        WithdrawalRequest memory request = $.withdrawalRequests[_msgSender()][token];


storage but not memory data allocation in such cases saves some gas.

Kolezhniuk added 3 commits December 21, 2024 22:38

Sponsor payment contract

f188932

Add deploy script and fixes

fd5d37e

Add deploy script and fixes

62f7390

Kolezhniuk requested review from demonsh, AndriianChestnykh, volodymyr-basiuk and vmidyllic December 23, 2024 07:50

Kolezhniuk marked this pull request as ready for review December 23, 2024 07:50

vmidyllic reviewed Dec 23, 2024

View reviewed changes

add comment

a1dfa8e

Kolezhniuk added 3 commits December 23, 2024 12:03

fix validToken modifier

7ef6f30

update storage variable

7345f44

Replace contract check verification

11daab8

volodymyr-basiuk reviewed Dec 23, 2024

View reviewed changes

Change modifier and rename const

1bdb931

AndriianChestnykh reviewed Dec 31, 2024

View reviewed changes

Fix comments

3940d4d

Sponsor payment #326

Are you sure you want to change the base?

Sponsor payment #326

Conversation

Kolezhniuk commented Dec 23, 2024 • edited Loading

Some details:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12584161757

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kolezhniuk commented Dec 23, 2024 •

edited

Loading

coveralls commented Dec 23, 2024 •

edited

Loading