-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[confidential-transfer] Add transfer with fee proof generation and extraction #6945
[confidential-transfer] Add transfer with fee proof generation and extraction #6945
Conversation
cae9c4b
to
ac19d52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just tiny things
test_transfer_with_fee_proof_validity(100, 100, 5, 10); | ||
test_transfer_with_fee_proof_validity(100, 100, 5, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not use big numbers like with the transfer correctness test? ie 2^16 -1, 2^48 -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is no specific reason. I had a bunch of tests and it seemed like the test was becoming quite slow and the transfer amount was already somewhat tested for transfer without fee tests, so I thought I would just include the smaller ones. Now, I did add the tests for big numbers and used (5, 10)
and (5, 1)
for the (basis point, max fee)
parameters. I can also add more edge case fee parameters too if you see fit.
( | ||
CiphertextCommitmentEqualityProofData, | ||
BatchedGroupedCiphertext3HandlesValidityProofData, | ||
PercentageWithCapProofData, | ||
BatchedGroupedCiphertext2HandlesValidityProofData, | ||
BatchedRangeProofU256Data, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: normally we should make this into a struct, but I see that the rest of the crate is using tuples, so this can stay. For the future though, would it be worth using structs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is a good point and I think we can certainly organize it using a structs. Since this issue is shared with transfer without fee, I'll address it in a separate PR.
token/confidential-transfer/proof-generation/src/transfer_with_fee.rs
Outdated
Show resolved
Hide resolved
ristretto::multiply_ristretto(&transfer_fee_basis_points_scalar, &transfer_amount_point) | ||
.ok_or(TokenProofExtractionError::CurveArithmetic)?; | ||
|
||
const MAX_FEE_BASIS_POINTS: u64 = 10_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You can remove this and use the one defined at the top of the file instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thanks for the catch. Fixed!
Co-authored-by: Jon C <me@jonc.dev>
e7b7900
to
6798ef0
Compare
Problem
The logic to generate proof data for transfer with fee have not been added to the repo yet.
Summary
Add the logic to generate the proof data. This proof is quite technical, but it essentially follows the existing logic in token client.