fix isPaymentDone verification - verify by recovered address not reci… #314
Conversation
Pull Request Test Coverage Report for Build 11778291936Details
💛 - Coveralls |
contracts/payment/MCPayment.sol
Outdated
| @@ -112,8 +112,8 @@ contract MCPayment is Ownable2StepUpgradeable, EIP712Upgradeable { | |||
| Iden3PaymentRailsRequestV1 memory paymentData, | |||
| bytes memory signature | |||
| ) external payable { | |||
| verifyIden3PaymentRailsRequestV1Signature(paymentData, signature); | |||
| bytes32 paymentId = keccak256(abi.encode(paymentData.recipient, paymentData.nonce)); | |||
| address recoverd = recoverIden3PaymentRailsRequestV1Signature(paymentData, signature); | |||
There was a problem hiding this comment.
recovered, or you can use signer, which has somewhat better readability
There was a problem hiding this comment.
signer better name, changed
contracts/payment/MCPayment.sol
Outdated
| } | ||
|
|
||
| function isPaymentDone(address recipient, uint256 nonce) external view returns (bool) { | ||
| function isPaymentDone(address signer, uint256 nonce) external view returns (bool) { |
There was a problem hiding this comment.
IMHO recipient was a better name as for the purpose of this API, and better understood by whoever reads it first time. Signer is related to details of implementation, which are lower than this level.
P.S. After reading the PR till the end I realised that it's not recipient but issuer which is the payment record is accounted under. However can you just replace signer with 'issuer' is a question? If the protocol evolve in a way that it's not only issuer entity, which signs then there should be different name. Anyway, signer doesn't look like the best option. It confuses a new reader. A one can think that signer is the one who pays and signs something. So better to figure out better naming.
There was a problem hiding this comment.
issuer now always sign the message, so we can use issuer
test/payment/mc-payment.test.ts
Outdated
| console.log("Verification Gas: " + verifyGas); | ||
| await payment | ||
| const recovered = await payment |
There was a problem hiding this comment.
Again signer is better here than recovered, I think
test/payment/mc-payment.test.ts
Outdated
| const verifyGas = await payment | ||
| .connect(userSigner) | ||
| .verifyIden3PaymentRailsRequestV1Signature.estimateGas(paymentData, signature); | ||
| .recoverIden3PaymentRailsRequestV1Signature.estimateGas(paymentData, signature); | ||
| console.log("Verification Gas: " + verifyGas); |
There was a problem hiding this comment.
Do you need the gas verification in the tests?
There was a problem hiding this comment.
not really, removed
|
@volodymyr-basiuk Additionally, please add NatSpec comments everywhere in the new payment functionality and check if verification will be run in the deploy scripts at deployment time in the same way as it is run for other contracts |
No description provided.