add setUsername functionality

[yuriko627]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (yarn build) was run locally and any changes were pushed
• Lint (yarn lint --check) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Closes #39

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

[vimwitch]

The core logic is correct, just need to remove some of the stuff around it. This pr also needs tests for this function. Just testing to make sure a username can't be double registered and that the old username is freed.

I'm going to publish a new version of the unirep stuff to npm today so it should be easier to build/test.

[yuriko627]

I get this error for should fail if a username is double registered test

Error: cannot estimate gas; transaction may fail or may require manual gas limit

I saw this ethereum/solidity#13159, but I assumed it's not ok to add allowUnlimitedContractSize: true. Is there any other way to get around this error? Or this test should be written different way in the first place?

[vimwitch]

Error: cannot estimate gas; transaction may fail or may require manual gas limi

This is a really generic error that occurs anytime the transaction might revert. In this case you need to send the transaction from the admin account to set a username, otherwise the require(msg.sender == admin) line will revert.

Try something like this:

const accounts = await ethers.getSigners()
await unirepSocialContract.connect(accounts[0]).setUsername(
  config.NUM_EPOCH_KEY_NONCE_PER_EPOCH,
  newUsername,
  newUsername
)

You also don't need the explicit assertions for tx success. You can replace

const tx = await unirepSocialContract.setUsername(
  config.NUM_EPOCH_KEY_NONCE_PER_EPOCH,
  newUsername,
  newUsername
)
const receipt = await tx.wait()
expect(receipt.status).equal(0)

With

await unirepSocialContract.setUsername(
  config.NUM_EPOCH_KEY_NONCE_PER_EPOCH,
  newUsername,
  newUsername
).then(t => t.wait())

The wait promise will throw an error if the transaction is not successful.

[vimwitch]

Ah I totally forgot something for this PR. We need to actually attest to the epoch key to give the key the username! This functionality is blocked by #30 because you need the attestation function. You'll use the submitAttestation function in the Unirep contract. Here is an example use. You'll want positive rep and negative rep to be 0, and the graffiti should be the new username.

You can rebase this branch on top of unirep-update now if you like, or you can wait for the pr to be merged.

[yuriko627]

I added submitAttestation part in the setUsername function, but submitAttestation function call failed to execute in the test.

[vimwitch]

This file still needs to be removed

[vimwitch]

@yuriko627 the files just need to be linted then this lgtm

[vimwitch]

This is just a note, instead of using like tx1/tx2 variables you can put this logic inside a block like this:

...
const newUsername4 = genRandomSalt().valueOf()
const accounts = await ethers.getSigners()
{
  const tx = await unirepSocialContract....
  const receipt = await tx.wait()
  expect(receipt.status)...
}
{
  const tx = await unirepSocialContract....
  const receipt = await tx.wait()
  expect(receipt.status)...
}

This bracket blocks can exist anywhere in the code, not just around if or for or functions or other flow control structures. Then the variables are scoped to that block and gone after.

[vivianjeng]

I think epochKey verification should be either in the backend or in the contract.
But it can be in another PR.
This PR LGTM.

[vivianjeng]

I am not sure whether epochKey should be verified on-chain so that it will not be easily changed by other users (if we don't want an admin to control)