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

[feat] Add GoodDollar whitelist #10

Open
julienbrg opened this issue Apr 24, 2023 · 17 comments
Open

[feat] Add GoodDollar whitelist #10

julienbrg opened this issue Apr 24, 2023 · 17 comments
Assignees
Labels
celo enhancement New feature or request

Comments

@julienbrg
Copy link
Contributor

No description provided.

@julienbrg julienbrg added the enhancement New feature or request label Apr 24, 2023
@julienbrg julienbrg added this to the v0.1.0 milestone Apr 24, 2023
@julienbrg julienbrg self-assigned this Apr 24, 2023
@julienbrg julienbrg modified the milestones: v0.1.0, v0.2.0 May 5, 2023
@bertux bertux assigned bertux and unassigned julienbrg May 14, 2023
@Shloyem Shloyem assigned Shloyem and unassigned bertux and Shloyem May 18, 2023
@Shloyem
Copy link
Collaborator

Shloyem commented May 21, 2023

IdentityV2 as seen in GD documentation or Github is the contract in charge of managing whitelisted addresses.
isWhitelisted(address) returns a bool if yes or no.
In the GD contracts docs it directs to Identity contracts deployed on Mainnet, Fuse (Old Fuse), and Celo.

@bertux
Copy link
Contributor

bertux commented May 23, 2023

Let's investigate https://github.com/GoodDollar/GoodProtocol/blob/master/releases/deployment.json by looking at networkId fields

@julienbrg
Copy link
Contributor Author

@Shloyem
Copy link
Collaborator

Shloyem commented May 23, 2023

The code change will be like in GD claiming contract UBIScheme.sol Claim function previous version
require( IIdentity(nameService.getAddress("IDENTITY")).isWhitelisted(msg.sender), "UBIScheme: not whitelisted" );

It was manually checked to work, and new UBIScheme claim is not needed - it checks if sender isWhiteListed or Identity contract itself, and holds root for further root actions
address whitelistedRoot = IIdentityV2(nameService.getAddress("IDENTITY")) .getWhitelistedRoot(msg.sender); require(whitelistedRoot != address(0), "UBIScheme: not whitelisted");

Moving on from here

@julienbrg
Copy link
Contributor Author

The code change will be like in GD claiming contract UBIScheme.sol Claim function previous version require( IIdentity(nameService.getAddress("IDENTITY")).isWhitelisted(msg.sender), "UBIScheme: not whitelisted" );

Yes, exactly.

It will work fine on whatever network where we actually have a GD whitelist availaible. @bertux @Shloyem What can we do for other networks and tests?

@julienbrg
Copy link
Contributor Author

@Shloyem We mentioned this one with @bertux today: we'll just optimize for Celo and Fuse, basically forget about testnets, and get inspired by GD best practices for the unit tests.

@Shloyem
Copy link
Collaborator

Shloyem commented Jun 8, 2023

As we later discussed on Telegram, it's first tested on Celo Alfajores testnet with mocks:
gCFA with the change -> That uses NameServiceMock -> That uses IdentityMock (allows to add or remove addresses from whitelist easily).

  1. Attempt to call gCFA.depositFor would revert with "Fail with error 'UBIScheme: not whitelisted'"
  2. Whitelisted my address on IdentityMock
  3. Approved the amount on EURMock
  4. gCFA.depositFor passed

Next thing I'll check on Celo either Mainnet or simulate via Tenderly

@julienbrg
Copy link
Contributor Author

From what I saw on this branch 10-feat-add-gooddollar-whitelist, we probably want to:

  • rebase from main branch
  • add a CELO_NAMESERVICE_ADDRESS in the .env.example file and in the deploy.ts script

@Shloyem Do you want me to do that + create a draft merge request?

@julienbrg
Copy link
Contributor Author

@Shloyem @bertux Also, I had a question for you guys: how would a user get whitelisted, please?

@julienbrg julienbrg modified the milestones: v0.2.0, v0.1.0 Jun 14, 2023
@Shloyem Shloyem linked a pull request Jun 15, 2023 that will close this issue
@julienbrg
Copy link
Contributor Author

julienbrg commented Jun 15, 2023

@Shloyem
Copy link
Collaborator

Shloyem commented Jun 16, 2023

From what I saw on this branch 10-feat-add-gooddollar-whitelist, we probably want to:

  • rebase from main branch
  • add a CELO_NAMESERVICE_ADDRESS in the .env.example file and in the deploy.ts script

@Shloyem Do you want me to do that + create a draft merge request?

Good ideas, done on draft pull request

@Shloyem
Copy link
Collaborator

Shloyem commented Jun 16, 2023

@Shloyem @bertux Also, I had a question for you guys: how would a user get whitelisted, please?

As far as I understand:

  1. Register to GoodDollar 2. Undergo it's face verification, so that no one user could claim with multiple account 3. Collect once in the last 30 days or so

Pros:

  • Use an existing working mechanism
  • "Forcing" users to enter the GoodDollar ecosystem, where besides gCFA they can also daily collect, transfer, convert, use tokens.

Cons:

  • Will need to introduce both gCFA idea and GoodDollar idea to new users.

But again this is my personal take, @bertux knows better:)

@julienbrg
Copy link
Contributor Author

Are we sure we need a setNameService function? Right now anyone can change the value of the nameService address, and that could make the deposit function unaccessible. @bertux Do we remove it or do we make it settable only by the DAO?

About this, we can just add a require in the setNameService function:

require(msg.sender == recoveryAddress, "Requires a community vote");

@julienbrg
Copy link
Contributor Author

@Shloyem @bertux Also, I had a question for you guys: how would a user get whitelisted, please?

As far as I understand:

  1. Register to GoodDollar 2. Undergo it's face verification, so that no one user could claim with multiple account 3. Collect once in the last 30 days or so

Thanks for your reply!

I'm definitely in favour of it. There's already a link pointing to GoodDapp claim (https://gooddapp.org/#/claim) in gCFA UI. But I need to know how can the UI detect if a user is whitelisted or not?

Upcoming version of the UI: https://gcfa-ui-staging.on.fleek.co/

@Shloyem I'm displaying this "You're currently not whitelisted, we need a Proof-of-Liveness which you can get here." but I need to call the isWhitelisted function to display correct info, right?

@Shloyem
Copy link
Collaborator

Shloyem commented Jun 16, 2023

@Shloyem @bertux Also, I had a question for you guys: how would a user get whitelisted, please?

As far as I understand:

  1. Register to GoodDollar 2. Undergo it's face verification, so that no one user could claim with multiple account 3. Collect once in the last 30 days or so

Thanks for your reply!

I'm definitely in favour of it. There's already a link pointing to GoodDapp claim (https://gooddapp.org/#/claim) in gCFA UI. But I need to know how can the UI detect if a user is whitelisted or not?

Upcoming version of the UI: https://gcfa-ui-staging.on.fleek.co/

Thanks for your replies as well, nice work

@Shloyem I'm displaying this "You're currently not whitelisted, we need a Proof-of-Liveness which you can get here." but I need to call the isWhitelisted function to display correct info, right?

Good idea, yes, described in the other item

@julienbrg julienbrg removed this from the v0.1.0 milestone Jun 18, 2023
@julienbrg julienbrg added the celo label Jun 19, 2023
@julienbrg
Copy link
Contributor Author

We need to:

  • Fix prettier (start with removing the prettier file)
  • Add the whitelist tests in the tests
  • Fix the ABI format: there should be a .abi file (json format)

@bertux
Copy link
Contributor

bertux commented Aug 2, 2023

We need to:

  • Fix prettier (start with removing the prettier file)
  • Add the whitelist tests in the tests
  • Fix the ABI format: there should be a .abi file (json format)

Let's focus on the first 2 points, the other can be improved later, it's already the same in the UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
celo enhancement New feature or request
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

3 participants