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

Allow for CIP8 that bypasses CIP3 #8360

Merged
merged 19 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 116 additions & 1 deletion packages/protocol/contracts/common/Accounts.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ contract Accounts is
);
bytes32 public eip712DomainSeparator;

// A per-account list of CIP8 storage roots, bypassing CIP3.
mapping(address => bytes[]) public offchainStorageRoots;

bytes32 constant ValidatorSigner = keccak256(abi.encodePacked("celo.org/core/validator"));
bytes32 constant AttestationSigner = keccak256(abi.encodePacked("celo.org/core/attestation"));
bytes32 constant VoteSigner = keccak256(abi.encodePacked("celo.org/core/vote"));
Expand All @@ -96,6 +99,8 @@ contract Accounts is
event AccountMetadataURLSet(address indexed account, string metadataURL);
event AccountWalletAddressSet(address indexed account, address walletAddress);
event AccountCreated(address indexed account);
event OffchainStorageRootAdded(address indexed account, bytes url);
event OffchainStorageRootRemoved(address indexed account, bytes url);

/**
* @notice Sets initialized == true on implementation contracts
Expand All @@ -108,7 +113,7 @@ contract Accounts is
* @return The storage, major, minor, and patch version of the contract.
*/
function getVersionNumber() external pure returns (uint256, uint256, uint256, uint256) {
return (1, 1, 2, 1);
return (1, 1, 3, 0);
}

/**
Expand Down Expand Up @@ -238,6 +243,66 @@ contract Accounts is
emit AccountMetadataURLSet(msg.sender, metadataURL);
}

/**
* @notice Adds a new CIP8 storage root.
* @param url The URL pointing to the offchain storage root.
*/
function addStorageRoot(bytes calldata url) external {
require(isAccount(msg.sender), "Unknown account");
offchainStorageRoots[msg.sender].push(url);
emit OffchainStorageRootAdded(msg.sender, url);
}

/**
* @notice Removes a CIP8 storage root.
* @param index The index of the storage root to be removed in the account's
* list of storage roots.
* @dev The order of storage roots may change after this operation (the last
* storage root will be moved to `index`), be aware of this if removing
* multiple storage roots at a time.
*/
function removeStorageRoot(uint256 index) external {
require(isAccount(msg.sender), "Unknown account");
require(index < offchainStorageRoots[msg.sender].length);
uint256 lastIndex = offchainStorageRoots[msg.sender].length - 1;
bytes memory url = offchainStorageRoots[msg.sender][index];
offchainStorageRoots[msg.sender][index] = offchainStorageRoots[msg.sender][lastIndex];
offchainStorageRoots[msg.sender].length--;
emit OffchainStorageRootRemoved(msg.sender, url);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think index should also be emitted here for cache invalidation

}

/**
* @notice Returns the full list of offchain storage roots for an account.
* @param account The account whose storage roots to return.
* @return List of storage root URLs.
*/
function getOffchainStorageRoots(address account)
external
view
returns (bytes memory, uint256[] memory)
{
require(isAccount(account), "Unknown account");
uint256 numberRoots = offchainStorageRoots[account].length;
uint256 totalLength = 0;
for (uint256 i = 0; i < numberRoots; i++) {
totalLength += offchainStorageRoots[account][i].length;
}

bytes memory concatenated = new bytes(totalLength);
uint256 lastIndex = 0;
uint256[] memory lengths = new uint256[](numberRoots);
Comment on lines +285 to +293
Copy link
Contributor

Choose a reason for hiding this comment

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

does storing the roots concatenated make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that would make adding/removing roots more complicated?

for (uint256 i = 0; i < numberRoots; i++) {
bytes storage root = offchainStorageRoots[account][i];
lengths[i] = root.length;
for (uint256 j = 0; j < lengths[i]; j++) {
concatenated[lastIndex] = root[j];
lastIndex++;
}
}

return (concatenated, lengths);
}

/**
* @notice Set the indexed signer for a specific role
* @param signer the address to set as default
Expand Down Expand Up @@ -857,6 +922,56 @@ contract Accounts is
return (sizes, data);
}

/**
* @notice Getter for the storage roots of multiple accounts.
* @param accountsToQuery The addresses of the accounts to get the storage roots for.
* @return (
* data - all strings concatenated
* rootLengths[] - the length of each string in `data` in bytes
* accountRootNumbers[] - the number of storage roots per account
* )
*/

function batchGetOffchainStorageRoots(address[] calldata accountsToQuery)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe lacking context but don't see why this is useful
I suppose to reduce number of round trips but the RPC supports batching calls too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe avoiding round trips was the main goal with this. If this is supported out-of-contract that would be ideal (especially given how complicated this function ended up being), let's discuss this with the wallet team that would be consuming this.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed that I can't really envision a specific use case for this, we can always batch from the wallet side

external
view
returns (bytes memory, uint256[] memory, uint256[] memory)
{
uint256[] memory accountRootNumbers = new uint256[](accountsToQuery.length);
uint256 totalRoots = 0;
for (uint256 i = 0; i < accountsToQuery.length; i++) {
require(isAccount(accountsToQuery[i]), "Unknown account");
accountRootNumbers[i] = offchainStorageRoots[accountsToQuery[i]].length;
totalRoots += accountRootNumbers[i];
}

uint256[] memory rootLengths = new uint256[](totalRoots);
uint256 totalLength = 0;
uint256 current = 0;

for (uint256 i = 0; i < accountsToQuery.length; i++) {
for (uint256 j = 0; j < accountRootNumbers[i]; j++) {
rootLengths[current] = offchainStorageRoots[accountsToQuery[i]][j].length;
totalLength += rootLengths[current];
current++;
}
}

bytes memory concatenated = new bytes(totalLength);
current = 0;
for (uint256 i = 0; i < accountsToQuery.length; i++) {
for (uint256 j = 0; j < accountRootNumbers[i]; j++) {
bytes storage root = offchainStorageRoots[accountsToQuery[i]][j];
for (uint256 k = 0; k < root.length; k++) {
concatenated[current] = root[k];
current++;
}
}
}

return (concatenated, rootLengths, accountRootNumbers);
}

/**
* @notice Getter for the data encryption key and version.
* @param account The address of the account to get the key for
Expand Down
5 changes: 5 additions & 0 deletions packages/protocol/specs/accounts.spec
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@ rule viewFunctionsDoNotRevert(method f) filtered { f ->
// some functions we ignore, and the reasons:
&& f.selector != hasAuthorizedSigner(address,string).selector // Calldatasize may not match
&& f.selector != batchGetMetadataURL(address[]).selector // Calldatasize may not match

// These require an account to exist
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

&& f.selector != getOffchainStorageRoots(address).selector
&& f.selector != batchGetOffchainStorageRoots(address[]).selector
&& f.selector != offchainStorageRoots(address,uint256).selector
} {
env e;
require e.msg.value == 0; // view functions are not payable
Expand Down
2 changes: 2 additions & 0 deletions packages/protocol/specs/accountsPrivileged.spec
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ definition knownAsNonPrivileged(method f) returns bool = false
|| f.selector == authorizeSigner(address,bytes32).selector
|| f.selector == setIndexedSigner(address,bytes32).selector
|| f.selector == setMetadataURL(string).selector
|| f.selector == addStorageRoot(bytes).selector
|| f.selector == removeStorageRoot(uint256).selector
|| f.selector == removeAttestationSigner().selector
|| f.selector == authorizeAttestationSigner(address,uint8,bytes32,bytes32).selector
|| f.selector == setAccount(string,bytes,address,uint8,bytes32,bytes32).selector
Expand Down
139 changes: 139 additions & 0 deletions packages/protocol/test/common/accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,25 @@ import {
RegistryContract,
} from 'types'
import { keccak256 } from 'web3-utils'

import BigNumber from 'bignumber.js'

const Accounts: AccountsContract = artifacts.require('Accounts')
const Registry: RegistryContract = artifacts.require('Registry')
const MockValidators: MockValidatorsContract = artifacts.require('MockValidators')

const assertStorageRoots = (rootsHex: string, lengths: BigNumber[], expectedRoots: string[]) => {
assert.equal(lengths.length, expectedRoots.length)
const roots = web3.utils.hexToUtf8(rootsHex)
let currentIndex = 0
expectedRoots.forEach((expectedRoot: string, i: number) => {
const root = roots.slice(currentIndex, currentIndex + lengths[i].toNumber())
currentIndex += lengths[i].toNumber()
assert.equal(root, expectedRoot)
})
assert.equal(roots.length, currentIndex)
}

contract('Accounts', (accounts: string[]) => {
let accountsInstance: AccountsInstance
let mockValidators: MockValidatorsInstance
Expand All @@ -26,6 +41,9 @@ contract('Accounts', (accounts: string[]) => {

const name = 'Account'
const metadataURL = 'https://www.celo.org'
const otherMetadataURL = 'https://clabs.co'
const storageRoot = web3.utils.utf8ToHex(metadataURL)
const otherStorageRoot = web3.utils.utf8ToHex(otherMetadataURL)
const dataEncryptionKey = '0x02f2f48ee19680706196e2e339e5da3491186e0c4c5030670656b0e01611111111'
const longDataEncryptionKey =
'0x04f2f48ee19680706196e2e339e5da3491186e0c4c5030670656b0e01611111111' +
Expand Down Expand Up @@ -343,6 +361,127 @@ contract('Accounts', (accounts: string[]) => {
})
})

describe('#addStorageRoot', () => {
describe('when the account has not been created', () => {
it('should revert', async () => {
await assertRevert(accountsInstance.addStorageRoot(storageRoot))
})
})

describe('when the account has been created', () => {
beforeEach(async () => {
await accountsInstance.createAccount()
})

it('adds a new storage root', async () => {
await accountsInstance.addStorageRoot(storageRoot)
const [rootsHex, lengths] = await accountsInstance.getOffchainStorageRoots(accounts[0])
assertStorageRoots(rootsHex, lengths, [metadataURL])
})

it('should emit the OffchainStorageRootAdded event', async () => {
const response = await accountsInstance.addStorageRoot(storageRoot)
assert.lengthOf(response.logs, 1)
const event = response.logs[0]
assertLogMatches2(event, {
event: 'OffchainStorageRootAdded',
args: { account: caller, url: storageRoot },
})
})

it('can add multiple storage roots', async () => {
await accountsInstance.addStorageRoot(storageRoot)
await accountsInstance.addStorageRoot(otherStorageRoot)
const [rootsHex, lengths] = await accountsInstance.getOffchainStorageRoots(accounts[0])
assertStorageRoots(rootsHex, lengths, [metadataURL, otherMetadataURL])
})
})
})

describe('#removeStorageRoot', () => {
describe('when the account has not been created', () => {
it('should revert', async () => {
await assertRevert(accountsInstance.removeStorageRoot(0))
})
})

describe('when the account has been created', () => {
beforeEach(async () => {
await accountsInstance.createAccount()
})

describe('when there are no storage roots', async () => {
it('should revert', async () => {
await assertRevert(accountsInstance.removeStorageRoot(0))
})
})

describe('when there are storage roots', async () => {
beforeEach(async () => {
await accountsInstance.addStorageRoot(storageRoot)
await accountsInstance.addStorageRoot(otherStorageRoot)
})

it('should remove one of the storage roots', async () => {
await accountsInstance.removeStorageRoot(0)
const [rootsHex, lengths] = await accountsInstance.getOffchainStorageRoots(accounts[0])
assertStorageRoots(rootsHex, lengths, [otherMetadataURL])
})

it('should remove a different storage root', async () => {
await accountsInstance.removeStorageRoot(1)
const [rootsHex, lengths] = await accountsInstance.getOffchainStorageRoots(accounts[0])
assertStorageRoots(rootsHex, lengths, [metadataURL])
})

it('should emit the OffchainStorageRootRemoved event', async () => {
const response = await accountsInstance.removeStorageRoot(0)
assert.lengthOf(response.logs, 1)
const event = response.logs[0]
assertLogMatches2(event, {
event: 'OffchainStorageRootRemoved',
args: { account: caller, url: storageRoot },
})
})
})
})
})

describe('#batchGetOffchainStorageRoots', () => {
it('returns storage roots for multiple accounts', async () => {
const metadataURLs1 = [metadataURL, otherMetadataURL]
const storageRoots1: string[] = [storageRoot, otherStorageRoot]
const metadataURLs2 = ['abc', 'def', 'ghi']
const storageRoots2: string[] = metadataURLs2.map(web3.utils.utf8ToHex)
await accountsInstance.createAccount()

await accountsInstance.createAccount({ from: accounts[1] })
for (const root of storageRoots1) {
await accountsInstance.addStorageRoot(root, { from: accounts[1] })
}

await accountsInstance.createAccount({ from: accounts[2] })
for (const root of storageRoots2) {
await accountsInstance.addStorageRoot(root, { from: accounts[2] })
}

const [
rootsHex,
lengths,
lengthLengths,
] = await accountsInstance.batchGetOffchainStorageRoots(accounts.slice(0, 3))

const roots = web3.utils.hexToUtf8(rootsHex)

const allURLs = metadataURLs1.concat(metadataURLs2)
const concatanatedURLs = allURLs.join('')

assert.equal(lengthLengths.length, 3)
assert.equal(lengths.length, 5)
assert.equal(roots, concatanatedURLs)
})
})

describe('#setName', () => {
describe('when the account has not been created', () => {
it('should revert', async () => {
Expand Down