[Feature] Spender permit feature #110
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1) Add new spendFromUserToWithPermit function, and add the foundry test. 2) Add the _requester parameter of the spendFromUserToWithPermit function, where _requester parameter is the address of strategy contract.
The difference between the old and new signature formats is as follows: The old signature format is: abi.encodePacked(r, s, v, bytes32(0), uint8(2)) The new signature format is: abi.encodePacked(r, s, v, uint8(2))
1) Remove spendFromUser and spendFromUserTo func. 2) Use the spendFromUserToWithPermit func, and transfer the token to RFQ first, then transfer to the taker, maker or feeCollector. 3) Add 2 spender parameters and 2 signatures to use spendFromUserToWithPermit func. 4) Modify foundry test to use 2 spender parameters and 2 signatures. 5) Modify foundry test to use new signature format: abi.encodePacked(r, s, v, uint8(sigType)); 6) Verify command: % forge test -vvv --match-path 'contracts/test/RFQ.t.sol'
Use struct of SpenderLibEIP712.SpendWithPermit in function parameters to increase readability and avoid future 'stake too deep' error.
Spender needs to make sure `msg.sender == requester`, so it will just replace it with `msg.sender` in `spendFromUserToWithPermit()` function.
1) Modify BPS_MAX variable and _createSpenderPermitFromOrder() function. 2) This Create Spender function returnss 2 SpendWithPermit variables and avoids setting global variables like in previous version.
The requester variable is used to verify that the request is sent from the correct strategy contract.
Use struct of SpenderLibEIP712.SpendWithPermit in function parameters to increase readability.
The replay protection check is done before isValidSignature() to avoid possible reentrancy attacks.
1) All parameters of taker/maker in call data are already in _order. 2) This reduces the number of parameters.
1) The taker/maker spendWithPermit are not used anywhere else in the fill() func, so move them into the _settle() func instead. 2) Fill in the taker/maker spendWithPermit data with _order data so that it doesn't need to be checked by require handling func. 3) Modified some comments on the usage of IERC20.safeTransfer() and spender.spendFromUserToWithPermit() func.
1) Modify the error message returned by the require handler that handles replay protection checks. 2) Modify the expected return error message for Foundry test. 3) Modify some comments on the usage of the require handler that handles expiry and requester checks.
Use only LibConstant.BPS_MAX variable in RFQ Foundry tests and added SafeMath library for unit16 variable.
…-compatible-permit-function Labs 940/spender adds backward compatible permit function
1) To prevent the signature from being used by another transaction in the same strategy contract, the spendWithPermit must contain the hash of the transaction or order the user wants to execute. 2) Replace salt in spendWithPermit with txHash since txHash itself contains salt value. 3) The value of txHash is the hash of the transaction, order or deposit hash calculated in each (AMM, RFQ, L2Deposit, LimitOrder, etc.) strategy contract. 4) Modified some comments on the usage of Spender.t.sol test file.
…hpermit Modify salt usage of spendwithpermit
1) To prevent the signature from being used by another transaction in the same strategy contract, the spendWithPermit must contain the hash of the transaction or order the user wants to execute. 2) Replace txHash in spendWithPermit with actionHash since actionHash will not be confused with blockchain's txHash. 3) The value of actionHash is the hash of the transaction, order or deposit hash calculated in each (AMM, RFQ, L2Deposit, LimitOrder, etc.) strategy contract.
…thpermit feat: spendWithPermit should contain transaction or order hash
RFQ, Spender contracts and their Foundry test contract changed to non named parameter type.
charlesjhongc
commented
Nov 9, 2022
contracts/test/Spender.t.sol
Outdated
| return keccak256(abi.encodePacked(EIP191_HEADER, EIP712_DOMAIN_SEPARATOR, structHash)); | ||
| } | ||
|
|
||
| function _signSpendWithPermit(uint256 privateKey, SpenderLibEIP712.SpendWithPermit memory spendWithPermit) internal returns (bytes memory sig) { |
1) Centralize the signSpendWithPermit function into Permit.sol to reduce the amount of code. 2) Currently only the RFQ.t.sol and Spender.t.sol files are modified.
When using the signSpendWithPermit() function, just input the EIP-712 Domain Separator instead of the entire Spender contract.
refac: move the signSpendWithPermit function in foundry test
1) Remove spendFromUser and spendFromUserTo func. 2) Reduce parameters of trade() function to avoid stack too deep error. 3) Add the takerAssetPermitSig parameter of the trade() function to authenticate the spendFromUserToWithPermit function. 4) Because AMMWrapper and AMMWrapperWithPath use the same _prepare() function, they need to be modified together. 5) Modify Foundry tests for AMMWrapper and AMMWrapperWithPath to use trade() function with takerAssetPermitSig parameter. 6) In Foundry tests, use curly braces to avoid stack too deep error.
1) Add underscore to the 'order' variable name to match the internal variable format. 2) Check pretty. 3) Remove unused BPS_MAX variable.
…nderFromUserWithPermit This merge if for update on PR#114 that merged into branch 'spender_permit_feature'
1) Change the internal function name from _prepare to _transferTakerAssetToAMM. 2) Modify some comments. 3) Delete unused test function.
…rFromUserWithPermit Modify l2deposit using spender from user with permit
1) Modify some comments to make them shorter. 2) Remove unused imported package. 3) Modify the test function from view to pure to prvent warning.
1) Use Named Parameters in AMM Foundry test to make code clearer. 2) Add address usage comments in AMMWrapper.sol to make clearer.
Amm with spender permit
The spendFromUser() function in the Spender.sol contract was replaced by the new spendFromUserToWithPermit() function, so the spendFromUser() was removed and the associated Foundry Test was also removed.
feat: remove spendFromUser() func in Spender.sol
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement permit feature on spender and replace original func calls in RFQ.