-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add more Auditor Tests #6255
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
base: ripple/confidential-transfer
Are you sure you want to change the base?
Add more Auditor Tests #6255
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ripple/confidential-transfer #6255 +/- ##
==============================================================
- Coverage 79.1% 79.0% -0.1%
==============================================================
Files 850 850
Lines 72015 72594 +579
Branches 8385 8416 +31
==============================================================
+ Hits 56975 57340 +365
- Misses 15040 15254 +214
🚀 New features to boost your workflow:
|
|
could the conflict be resolved? thanks |
| 0xAD, 0x0B, 0xE3, 0x91, 0x50, 0xDA, 0x2F, 0x75, 0xC6, 0xBD, 0x42}; | ||
|
|
||
| // a valid hardcoded ciphertext for testing | ||
| constexpr static unsigned char |
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.
I have a refactor in #6274
So we can get rid of this and call the getTrivialCiphertext() after that PR is merged
| .amt = 10, | ||
| .holderPubKey = mptAlice.getPubKey(bob), | ||
| .auditorEncryptedAmt = | ||
| Buffer{badCiphertext, ecGamalEncryptedTotalLength}, |
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.
https://github.com/XRPLF/rippled/pull/6274/files modified to use getBadCiphertext().
You can update it after https://github.com/XRPLF/rippled/pull/6274/files is merged
|
|
||
| mptAlice.generateKeyPair(alice); | ||
|
|
||
| mptAlice.set( |
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.
auditor key is invalid length, pub key is invalid have similar setup. We can just move the code of line 436 after 399.
| mptAlice.generateKeyPair(alice); | ||
|
|
||
| // Providing only auditor key should fail | ||
| mptAlice.set( |
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.
same here, we can move after line 399. The setup lines can be shared
| {.account = alice, | ||
| .auditorPubKey = mptAlice.getPubKey(alice), | ||
| .err = temMALFORMED}); | ||
| } |
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.
We should also test:
if (hasHolder && (hasIssuerElGamalKey || hasAuditorElGamalKey))
return temMALFORMED;
and this can also be added after line 399
| .auditorPubKey = mptAlice.getPubKey(auditor)}); | ||
|
|
||
| // no auditor encrypted amt provided | ||
| mptAlice.convert( |
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.
In the test framework, even if the auditor's encrypted amt is not provided, it will fill in the auditor's encrypted amount automatically: https://github.com/XRPLF/rippled/blob/ripple/confidential-transfer/src/test/jtx/impl/mpt.cpp#L1015C5-L1015C30
Could you double check if this is failing at the expected line? I think we need some logic in test framework to force it not auto filling this field.
| // We set the auditor key, but auditorEncryptedAmt is missing | ||
| // Note:we need to create a new MPTTester to reset the state | ||
| { | ||
| Env env2{*this, features}; |
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.
the naming should remove 2
| std::optional<Buffer>& auditorCiphertext, | ||
| Buffer& blindingFactor) const | ||
| Buffer& blindingFactor, | ||
| bool fillAuditorEncryptedAmt) const |
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.
why is this fillAuditorEncryptedAmt param needed? which test would fail otherwise?
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.
It fails the test where we want to test for if (requiresAuditor != hasAuditor)
Essentially, fillConversionCiphertexts makes sure if the field auditor is present, it fills the auditorEncryptedAmt, so I created a bool field to prevent this
| } | ||
|
|
||
| // cannot convert if auditor key is set, but auditor amount is not | ||
| // provided |
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.
should also test MPTokenIssuanceSet where the issuer has already set IssuerPublicKey. Then they try to set AuditorPublicKey, which should fail.
13672c5 to
c38a3b3
Compare
c38a3b3 to
3a86ae6
Compare
High Level Overview of Change
Adds more unit tests for Auditor feature in Confidential Transfer
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)