Support claim signatures and support Keybase claims #1575
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1575 +/- ##
=========================================
Coverage ? 74.15%
=========================================
Files ? 287
Lines ? 7652
Branches ? 667
=========================================
Hits ? 5674
Misses ? 1865
Partials ? 113
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
lefts some comments, but in general everything is good.
Maybe we can add tests to the cli commands, they don't seem to be as "small" as usual.
For testing cli commands, i've added support for them in another pr which is about to be merged. Basically you can run a command using ganache/devchain same as contractkit
| @@ -0,0 +1,52 @@ | |||
| import { spawn, SpawnOptions } from 'child_process' | |||
|
|
|||
| export function execCmd( | |||
There was a problem hiding this comment.
at this point... having so many instances of this fn, shouldn't we move it to celo-utils?
There was a problem hiding this comment.
We should, but I'm not sure I want to be the person transforming the codebase, nor prescribe which version should be the one to rule them all
packages/cli/src/utils/cli.ts
Outdated
| @@ -59,3 +59,8 @@ export function failWith(msg: string): never { | |||
| console.error(msg) | |||
| return process.exit(1) | |||
| } | |||
|
|
|||
| export async function binaryPrompt(promptMessage: string) { | |||
| const resp = await cli.prompt(promptMessage) | |||
There was a problem hiding this comment.
maybe add a fixed "[y/yes, n/no]" as a prompt suffix?
There was a problem hiding this comment.
i don't if oclif has it, but a good library for this kind of things is inquirer.js
| } | ||
|
|
||
| test('account:create-metadata cmd', async () => { | ||
| const newFilePath = `${tmpdir()}/newfile.json` |
There was a problem hiding this comment.
don't you need to remove the file if already present?
There was a problem hiding this comment.
Yeah, but I'm just assuming that it does not exist :)
| test('account:create-metadata cmd', async () => { | ||
| const newFilePath = `${tmpdir()}/newfile.json` | ||
| await CreateMetadata.run(['--from', account, newFilePath]) | ||
| const res = JSON.parse(readFileSync(newFilePath).toString()) |
There was a problem hiding this comment.
something that you can do is use jest snapshot testing which is a nice way of doing a quick test that asserts the content remains the same over different runs
There was a problem hiding this comment.
I'm gonna leave that for a future PR :)
|
But the second time you run the test it will
…On Thu, 7 Nov 2019 at 17:45 Nam Chu Hoai ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/cli/src/commands/account/claims.test.ts
<#1575 (comment)>
:
> + })
+
+ describe('Modifying the metadata file', () => {
+ const emptyFilePath = `${tmpdir()}/metadata.json`
+ const generateEmptyMetadataFile = () => {
+ writeFileSync(emptyFilePath, IdentityMetadataWrapper.fromEmpty(account))
+ }
+
+ const readFile = () => {
+ return IdentityMetadataWrapper.fromFile(emptyFilePath)
+ }
+
+ test('account:create-metadata cmd', async () => {
+ const newFilePath = `${tmpdir()}/newfile.json`
+ await CreateMetadata.run(['--from', account, newFilePath])
+ const res = JSON.parse(readFileSync(newFilePath).toString())
I'm gonna leave that for a future PR :)
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1575?email_source=notifications&email_token=AAFAHFBRYX7LXPI2MUGEDLTQSTADTA5CNFSM4JIX27LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKZVM4Q#discussion_r343957863>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFAHFEDHLVHY5LQ66YHSYTQSTADTANCNFSM4JIX27LA>
.
|
* master: (56 commits) Adjust e2e transfer and governance tests to match new fee distribution and eliminate ProposerFraction (#1585) [Wallet] Add more local currencies (#1698) Switch to correct cluster when fauceting (#1687) [Wallet] Use the country of the phone number for determining the default local currency (#1684) [Wallet] Limit QR code scanner to 1 code per second (#1676) Update Dark backgrounds text color (#1677) Remove integration sync test Minor attestation service fixes (#1680) [wallet] Fixed Native phone picker Use native API instead (#1669) Fix token addresses for notification service (#1674) Add golang to setup docs [wallet] Hide invite education copy after invite was redeemed (#1670) [Wallet] Add spinner, timer, and tip text to Verification input screen (#1656) [Wallet] Fix app deprecation check mechanism (#1358) Point end-to-end governance test back to master (#1665) Add EpochRewards smart contract to calculate epoch rewards and payments (#1558) Optimized Attestation view calls and removal of the reveal TX (#1578) Support claim signatures and support Keybase claims (#1575) [Wallet] Add timestamp to top banner messages (#1657) Export geth metrics on VM testnet (#1351) ... # Conflicts: # yarn.lock
Description
This PR adds the ability to claim Keybase accounts. It also requires claims to be signed by the claiming account, and that is enforced during the parsing of metadata.
A user can claim a keybase account by running:
celocli account:claim-keybase ./namtest.json --username nambrot --from 0x13897F87A0aB80824fa85600b935dAF579b69157If the keybase CLI is installed, we will automatically attempt to upload the proof to the keybase file system, otherwise, we'll print manual instructions such as:
Tested
Related issues