Refactor to Accounts.sol #1392

nambrot · 2019-10-17T21:41:25Z

Description

This PR extracts common account/authorization logic into a single smart contract

Tested

Unit tests

Related issues

Fixes Developers SBAT reference an Accounts smart contract #1318

Backwards compatibility

This change will be breaking for all clients

codecov · 2019-10-18T21:29:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3c86fef). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1392   +/-   ##
=========================================
  Coverage          ?   73.61%           
=========================================
  Files             ?      277           
  Lines             ?     7443           
  Branches          ?      956           
=========================================
  Hits              ?     5479           
  Misses            ?     1852           
  Partials          ?      112

Flag	Coverage Δ
#mobile	`73.61% <ø> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c86fef...073a20b. Read the comment docs.

mcortesi · 2019-10-23T17:57:22Z

packages/protocol/contracts/common/interfaces/IAccounts.sol

+
+interface IAccounts {
+  function isAccount(address) external view returns (bool);
+  function getAccountFromVoteSigner(address) external view returns (address);


how do you feel about "voteSignerToAccount()" naming style?

I feel ok with any naming, happy to go with whatever y'all think is best @asaj

mcortesi · 2019-10-23T17:58:45Z

packages/protocol/contracts/common/interfaces/IAccounts.sol

+  function isAccount(address) external view returns (bool);
+  function getAccountFromVoteSigner(address) external view returns (address);
+  function getAccountFromValidationSigner(address) external view returns (address);
+  function getValidationSignerFromAccount(address) external view returns (address);


being that we are in the "Accounts" contract, i feel unnecessary to have the FromAccount suffix here; how about getValidationSigner(account)?

mcortesi · 2019-10-23T17:59:06Z

packages/protocol/contracts/common/interfaces/IAccounts.sol

+  function getAccountFromValidationSigner(address) external view returns (address);
+  function getValidationSignerFromAccount(address) external view returns (address);
+  function getAccountFromAttestationSigner(address) external view returns (address);
+  function getAttestationSignerFromAccount(address) external view returns (address);


mcortesi · 2019-10-23T18:29:47Z

packages/contractkit/src/wrappers/Accounts.ts

+   * @param attestationSigner The address of the signing key to authorize.
+   * @return A CeloTransactionObject
+   */
+  async authorizeAttestationSigner(


For a future PR, we can have a authorizeSigner(role, account, signer)

mcortesi · 2019-10-23T18:32:25Z

packages/contractkit/src/wrappers/Accounts.ts

+   * @param account The address of the account.
+   * @return The address with which the account can vote.
+   */
+  getVoteSignerFromAccount: (account: string) => Promise<Address> = proxyCall(


for a future PR, maybe have a getSigner(role, account)

I was trying to think about how to make them work without having to effectively do a bunch of if statements that match on the role in solidity (which I imagine would make this more inefficient.). Did you have any rough idea here?

if you want to avoid the evil if, maybe you can use a Map<Role,Function>

const getters = { [Role.Voter]: this.contract.method.getVoteSignerFromAccount, ... } return getters[role](account)

would check about if it needs the bind(this.contract.methods)

Ah nvm, I thought this comment was on the solidity contract, not on the wrapper itself

I added it for the authorize functions, but didn't feel like it was appropriate here as just using the direct methods seemed preferrable to me

mcortesi · 2019-10-23T18:34:35Z

packages/cli/src/commands/identity/register-metadata.ts

@@ -22,7 +22,7 @@ export default class RegisterMetadata extends BaseCommand {
  async run() {
    const { flags } = this.parse(RegisterMetadata)
    this.kit.defaultAccount = flags.from
-    const attestations = await this.kit.contracts.getAttestations()
-    await displaySendTx('registerMetadata', attestations.setMetadataURL(flags.url))
+    const accounts = await this.kit.contracts.getAccounts()


Maybe this cmd should be part of accounts:...

mcortesi · 2019-10-23T18:35:14Z

packages/cli/src/commands/identity/get-metadata.ts

@@ -18,8 +18,8 @@ export default class GetMetadata extends BaseCommand {
  async run() {
    const { args } = this.parse(GetMetadata)
    const address = args.address
-    const attestations = await this.kit.contracts.getAttestations()
-    const metadataURL = await attestations.getMetadataURL(address)
+    const accounts = await this.kit.contracts.getAccounts()


maybe this should be part of accounts:...

* Adjust new interface from celo-org/celo-monorepo#1392 * Remove URL * Point e2e tests to nambrot/accounts

* master: (62 commits) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) [Wallet] Improve error handling around account creation and keystore ops (#1497) Add CI test for checking licenses and misc package.json cleanup (#1550) [Wallet] Implement SMS invite on iOS (#1541) CI: brings back to master (#1554) Validators: uses Ethereum address for proof of possession (#1494) Validate Attestation Requests (#1468) Rename hosted node references to forno (#1511) Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508) Added txpool family to geth apis. Sorted geth cmd options (#1462) [Wallet] Fix yarn dev command for running android (#1534) [Wallet] iOS info plist changes and version bump (#1539) ... # Conflicts: # yarn.lock

* master: (73 commits) Fix Ethstats Image reference (#1577) EU Cookies Behavior Change (#1447) [verifier] Upgrade to RN 61 (#1572) [Wallet] Update link styles and Implement VerificationEducationScreen (#1565) [wallet] Added native phone picker (#1310) [Wallet] Set up new verification screen skeletons (#1563) Bump e2e test migrate numbers where needed (#1567) [Wallet] Create new carousel component (#1555) [Wallet] Protect Backup Key and Safeguards with PIN (#1556) Increase ganache gas limit (#1569) Re-work locked gold requirements for validators and groups (#1474) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) ...

Refactor to Accounts.solc

0615954

nambrot requested review from asaj and m-chrzan as code owners October 17, 2019 21:41

nambrot added 6 commits October 18, 2019 10:15

Fix protocol tests

19f5d22

Fix contractkit wrappers

2907193

Add Accounts Wrapper

c7e5f06

Add Accounts to protocol build

a82e01a

Fix tests

efafab2

Fix CLI

577e0e3

nambrot requested a review from mcortesi as a code owner October 18, 2019 18:21

nambrot added 3 commits October 18, 2019 11:31

lint

74de8a8

Move from Attestations to Accounts

31e266c

Fix contractkit and migrations

1d93b8d

nambrot added 8 commits October 18, 2019 15:04

Fix CLI

b458228

Fix Celotool

3f12abf

Fix contract deploy

fae958d

Merge branch 'master' into nambrot/accounts

7a44122

Rename towards signing keys

b7aea10

Merge branch 'master' into nambrot/accounts

093a6f6

Fix Accounts tests

71ac441

Adjust CLI

464eb78

nambrot assigned asaj Oct 23, 2019

Fix CK

143ab38

mcortesi reviewed Oct 23, 2019

View reviewed changes

nambrot assigned mcortesi Oct 23, 2019

mcortesi reviewed Oct 23, 2019

View reviewed changes

mcortesi approved these changes Oct 23, 2019

View reviewed changes

Move CLI commands to account namespace

10fbdc4

asaj assigned nambrot and unassigned asaj Oct 30, 2019

mrsmkl mentioned this pull request Oct 30, 2019

Devs SBAT to know when a new account is registered at LockedGold #1404

Closed

PR comments

69992e4

mrsmkl mentioned this pull request Oct 30, 2019

Add celo account celo-org/blockscout#22

Merged

5 tasks

Rename

61465fa

nambrot added a commit to celo-org/celo-blockchain that referenced this pull request Oct 31, 2019

Adjust new interface from celo-org/celo-monorepo#1392

14b5cb1

nambrot mentioned this pull request Oct 31, 2019

Adjust new interface from Accounts.sol celo-org/celo-blockchain#560

Merged

nambrot added 10 commits October 31, 2019 10:56

Fix governance test

c27f1b1

Temporarily adjust for e2e test

9d9119e

Merge branch 'master' into nambrot/accounts

c81acfe

Generate address

eee94ec

Use stringIdentity

1e796d2

Fix merge conflict

e76c84c

Point to nambrot/accounts for e2e tests

b0d55aa

Merge branch 'master' into nambrot/accounts

4d33fc5

Merge branch 'master' into nambrot/accounts

0682ffe

Merge branch 'master' into nambrot/accounts

073a20b

nambrot merged commit 9c6495d into master Oct 31, 2019

nambrot added a commit to celo-org/celo-blockchain that referenced this pull request Oct 31, 2019

Adjust new interface from Accounts.sol (#560)

6d1c875

* Adjust new interface from celo-org/celo-monorepo#1392 * Remove URL * Point e2e tests to nambrot/accounts

nambrot mentioned this pull request Oct 31, 2019

Point e2e tests back #1562

Merged

mcortesi deleted the nambrot/accounts branch December 4, 2019 16:48

lili2311 mentioned this pull request Jul 11, 2023

[Snyk-dev] Fix for 6 vulnerabilities lili2311/lerna-celo-monorepo#69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to Accounts.sol #1392

Refactor to Accounts.sol #1392

nambrot commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 18, 2019 •

edited

Loading

mcortesi Oct 23, 2019

nambrot Oct 24, 2019

asaj Oct 25, 2019

mcortesi Oct 23, 2019

asaj Oct 30, 2019

mcortesi Oct 23, 2019

mcortesi Oct 23, 2019

mcortesi Oct 23, 2019 •

edited

Loading

nambrot Oct 23, 2019

mcortesi Oct 23, 2019

nambrot Oct 23, 2019

nambrot Oct 23, 2019

mcortesi Oct 23, 2019

mcortesi Oct 23, 2019

Refactor to Accounts.sol #1392

Refactor to Accounts.sol #1392

Conversation

nambrot commented Oct 17, 2019 • edited Loading

Description

Tested

Related issues

Backwards compatibility

codecov bot commented Oct 18, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcortesi Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nambrot commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 18, 2019 •

edited

Loading

mcortesi Oct 23, 2019 •

edited

Loading