Skip to content
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

Add verite credential support #69

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Add verite credential support #69

merged 2 commits into from
Oct 28, 2022

Conversation

venables
Copy link
Contributor

No description provided.

string schema; // indicator of the type of verification result
address subject; // address of the subject of the verification
uint256 expiration; // expiration of verification (may or may not be expiration of the VC)
string verifier_verification_id; // Unique ID from the verifier
Copy link
Contributor Author

@venables venables Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new field -- based on the documentation, it appears Circle added it for their API

bricestacey
bricestacey previously approved these changes Oct 28, 2022
@@ -60,32 +110,144 @@ contract PoolAccessControl is IPoolAccessControl {
* @inheritdoc IPoolAccessControl
*/
function isValidLender(address addr) external view returns (bool) {
return _allowedLenders[addr];
return
_tosRegistry.hasAccepted(addr) &&
Copy link
Contributor

@ams9198 ams9198 Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ToS acceptance is one-way (can't be revoked, at least currently), do we need to check it again here? I think we check on write for both allowList and Verite? Can't hurt to leave it in, but just double-checking my understanding is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right! I forgot you intentionally left it out here. I'll update

@@ -5,6 +5,8 @@ import "./interfaces/IPoolAccessControl.sol";
import "./interfaces/IPermissionedServiceConfiguration.sol";
import "./interfaces/IToSAcceptanceRegistry.sol";
import "../interfaces/IPool.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that it got bumped out of draft, at least in GH: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol. I think we're on the latest though, which doesn't seem to have it yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i was looking too and it appears its only in the 4.8.0-rc, not yet mainline

ams9198
ams9198 previously approved these changes Oct 28, 2022
We check whenever adding an address, so we do not need to use the cycles
checking here every time
@venables venables dismissed stale reviews from ams9198 and bricestacey via 80bfc23 October 28, 2022 20:02
@venables venables merged commit 8377b5e into circlefin:master Oct 28, 2022
@venables venables deleted the venables/verite branch October 28, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants