Skip to content

Commit

Permalink
refactor: eth address tech debt cleanup (#4442)
Browse files Browse the repository at this point in the history
We used to serialize EthAddress as 32 bytes because we needed that for
compatibility with CBINDs. Given that we no longer use the C++ code this
is no longer necessary.

**Note**: decided to not completely nuke `EthAddress.toBuffer32()`
because that would require updating how we compute calldata/body hash
and it makes sense to do that once we'll do that in #3938 (linked it in
the issue).
  • Loading branch information
benesjan committed Feb 6, 2024
1 parent 19e03ad commit 153989f
Show file tree
Hide file tree
Showing 19 changed files with 596 additions and 683 deletions.
2 changes: 1 addition & 1 deletion yarn-project/circuit-types/src/contract_data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export class ContractData {
* @returns Encoded buffer.
*/
public toBuffer(): Buffer {
return serializeToBuffer(this.contractAddress, this.portalContractAddress.toBuffer20());
return serializeToBuffer(this.contractAddress, this.portalContractAddress);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/circuit-types/src/l1_to_l2_message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ export class L1Actor {

static fromBuffer(buffer: Buffer | BufferReader): L1Actor {
const reader = BufferReader.asReader(buffer);
const ethAddr = new EthAddress(reader.readBytes(32));
const ethAddr = reader.readObject(EthAddress);
const chainId = reader.readNumber();
return new L1Actor(ethAddr, chainId);
}
Expand Down
1 change: 1 addition & 0 deletions yarn-project/circuit-types/src/l2_block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ export class L2Block {
newL2ToL1MsgsBuffer,
this.newContracts[i].toBuffer(),
this.newContractData[i].contractAddress.toBuffer(),
// TODO(#3938): make portal address 20 bytes here when updating the hashing
this.newContractData[i].portalContractAddress.toBuffer32(),
encryptedLogsHashKernel0,
unencryptedLogsHashKernel0,
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/circuits.js/src/abis/abis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { times } from '@aztec/foundation/collection';

import {
AztecAddress,
EthAddress,
Fr,
FunctionData,
FunctionLeafPreimage,
Expand Down Expand Up @@ -143,7 +144,7 @@ describe('abis', () => {
});

it('computes zero contract leaf', () => {
const cd = new NewContractData(AztecAddress.ZERO, AztecAddress.ZERO, new Fr(0n));
const cd = new NewContractData(AztecAddress.ZERO, EthAddress.ZERO, new Fr(0n));
const res = computeContractLeaf(cd);
expect(res).toMatchSnapshot();
});
Expand Down
25 changes: 8 additions & 17 deletions yarn-project/circuits.js/src/structs/call_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,8 @@ import { FieldsOf } from '@aztec/foundation/types';

/**
* Call context.
* @see abis/call_context.hpp
*/
export class CallContext {
/**
* Address of the portal contract to the storage contract.
*/
public portalContractAddress: EthAddress;
constructor(
/**
* Address of the account which represents the entity who invoked the call.
Expand All @@ -27,9 +22,8 @@ export class CallContext {
public storageContractAddress: AztecAddress,
/**
* Address of the portal contract to the storage contract.
* Union type is a kludge until C++ has an eth address type.
*/
portalContractAddress: EthAddress | Fr,
public portalContractAddress: EthAddress,
/**
* Function selector of the function being called.
*/
Expand All @@ -50,10 +44,7 @@ export class CallContext {
* The start side effect counter for this call context.
*/
public startSideEffectCounter: number,
) {
this.portalContractAddress =
portalContractAddress instanceof EthAddress ? portalContractAddress : EthAddress.fromField(portalContractAddress);
}
) {}

/**
* Returns a new instance of CallContext with zero msg sender, storage contract address and portal contract address.
Expand All @@ -63,7 +54,7 @@ export class CallContext {
return new CallContext(
AztecAddress.ZERO,
AztecAddress.ZERO,
Fr.ZERO,
EthAddress.ZERO,
FunctionSelector.empty(),
false,
false,
Expand Down Expand Up @@ -119,10 +110,10 @@ export class CallContext {
static fromBuffer(buffer: Buffer | BufferReader) {
const reader = BufferReader.asReader(buffer);
return new CallContext(
new AztecAddress(reader.readBytes(32)),
new AztecAddress(reader.readBytes(32)),
new EthAddress(reader.readBytes(32)),
FunctionSelector.fromBuffer(reader.readBytes(4)),
reader.readObject(AztecAddress),
reader.readObject(AztecAddress),
reader.readObject(EthAddress),
reader.readObject(FunctionSelector),
reader.readBoolean(),
reader.readBoolean(),
reader.readBoolean(),
Expand All @@ -135,7 +126,7 @@ export class CallContext {
return new CallContext(
reader.readObject(AztecAddress),
reader.readObject(AztecAddress),
reader.readField(),
reader.readObject(EthAddress),
reader.readObject(FunctionSelector),
reader.readBoolean(),
reader.readBoolean(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,18 @@ export class ContractDeploymentData {
Fr.fromBuffer(reader),
Fr.fromBuffer(reader),
Fr.fromBuffer(reader),
new EthAddress(reader.readBytes(32)),
reader.readObject(EthAddress),
);
}

static fromFields(fields: Fr[] | FieldReader): ContractDeploymentData {
const reader = FieldReader.asReader(fields);

const publicKey = Point.fromFields(reader);
const publicKey = reader.readObject(Point);
const initializationHash = reader.readField();
const contractClassId = reader.readField();
const contractAddressSalt = reader.readField();
const portalContractAddress = new EthAddress(reader.readField().toBuffer());
const portalContractAddress = reader.readObject(EthAddress);

return new ContractDeploymentData(
publicKey,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,29 +33,20 @@ import { SideEffect, SideEffectLinkedToNoteHash } from '../side_effects.js';
* Note: Not to be confused with `ContractDeploymentData`.
*/
export class NewContractData {
/**
* Ethereum address of the portal contract on L1.
*/
public portalContractAddress: EthAddress;
constructor(
/**
* Aztec address of the contract.
*/
public contractAddress: AztecAddress,
/**
* Ethereum address of the portal contract on L1.
* TODO(AD): refactor this later
* currently there is a kludge with circuits cpp as it emits an AztecAddress
*/
portalContractAddress: EthAddress | AztecAddress,
public portalContractAddress: EthAddress,
/**
* Contract class id.
*/
public contractClassId: Fr,
) {
// Handle circuits emitting this as an AztecAddress
this.portalContractAddress = new EthAddress(portalContractAddress.toBuffer());
}
) {}

toBuffer() {
return serializeToBuffer(this.contractAddress, this.portalContractAddress, this.contractClassId);
Expand All @@ -68,11 +59,7 @@ export class NewContractData {
*/
static fromBuffer(buffer: Buffer | BufferReader): NewContractData {
const reader = BufferReader.asReader(buffer);
return new NewContractData(
reader.readObject(AztecAddress),
new EthAddress(reader.readBytes(32)),
Fr.fromBuffer(reader),
);
return new NewContractData(reader.readObject(AztecAddress), reader.readObject(EthAddress), Fr.fromBuffer(reader));
}

static empty() {
Expand Down Expand Up @@ -157,7 +144,7 @@ export class OptionallyRevealedData {
Fr.fromBuffer(reader),
reader.readObject(FunctionData),
Fr.fromBuffer(reader),
new EthAddress(reader.readBytes(32)),
reader.readObject(EthAddress),
reader.readBoolean(),
reader.readBoolean(),
reader.readBoolean(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
// TODO(#4411): this is mostly redundant. Nuke this
import {
makeCallRequest,
makeFinalAccumulatedData,
makeNewSideEffect,
makePreviousKernelData,
makePrivateCallData,
makePrivateCircuitPublicInputs,
makePrivateKernelInputsInner,
makePrivateKernelPublicInputsFinal,
} from '../../tests/factories.js';
import {
CallRequest,
FinalAccumulatedData,
PreviousKernelData,
PrivateCircuitPublicInputs,
PrivateKernelPublicInputsFinal,
SideEffect,
} from '../index.js';
Expand Down Expand Up @@ -74,11 +73,3 @@ describe('CallRequest', () => {
expect(CallRequest.fromBuffer(buf)).toEqual(fad);
});
});

describe('Private Circuit Public Inputs', () => {
it('convert to and from buffer', () => {
const pkpi = makePrivateCircuitPublicInputs();
const buf = pkpi.toBuffer();
expect(PrivateCircuitPublicInputs.fromBuffer(buf)).toEqual(pkpi);
});
});
12 changes: 1 addition & 11 deletions yarn-project/foundation/src/eth-address/eth_address.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ describe('address', () => {

it('should return correct buffer', () => {
const address = EthAddress.fromString('0xc6d9d2cd449a754c494264e1809c50e34d64562b');
expect(address.toBuffer20()).toEqual(Buffer.from('c6d9d2cD449A754c494264e1809c50e34D64562b', 'hex'));
expect(address.toBuffer()).toEqual(Buffer.from('c6d9d2cD449A754c494264e1809c50e34D64562b', 'hex'));
});

it('should return correct 32 byte buffer', () => {
Expand All @@ -18,16 +18,6 @@ describe('address', () => {
);
});

it('should create address from 32 byte buffer', () => {
const buffer = Buffer.from('000000000000000000000000c6d9d2cD449A754c494264e1809c50e34D64562b', 'hex');
expect(new EthAddress(buffer)).toEqual(EthAddress.fromString('0xc6d9d2cD449A754c494264e1809c50e34D64562b'));
});

it('should not create address from 32 byte buffer that does not start with 12 0 bytes', () => {
const buffer = Buffer.from('010000000000000000000000c6d9d2cD449A754c494264e1809c50e34D64562b', 'hex');
expect(() => new EthAddress(buffer)).toThrowError();
});

it('should have correct zero address', () => {
expect(EthAddress.ZERO.toString()).toBe('0x0000000000000000000000000000000000000000');
});
Expand Down
33 changes: 11 additions & 22 deletions yarn-project/foundation/src/eth-address/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { keccak256String } from '../crypto/keccak/index.js';
import { randomBytes } from '../crypto/random/index.js';
import { Fr } from '../fields/index.js';
import { BufferReader } from '../serialize/index.js';
import { BufferReader, FieldReader } from '../serialize/index.js';

/**
* Represents an Ethereum address as a 20-byte buffer and provides various utility methods
Expand All @@ -20,13 +20,7 @@ export class EthAddress {
public static ZERO = new EthAddress(Buffer.alloc(EthAddress.SIZE_IN_BYTES));

constructor(private buffer: Buffer) {
if (buffer.length === 32) {
if (!buffer.slice(0, 12).equals(Buffer.alloc(12))) {
throw new Error(`Invalid address buffer: ${buffer.toString('hex')}`);
} else {
this.buffer = buffer.slice(12);
}
} else if (buffer.length !== EthAddress.SIZE_IN_BYTES) {
if (buffer.length !== EthAddress.SIZE_IN_BYTES) {
throw new Error(`Expect buffer size to be ${EthAddress.SIZE_IN_BYTES}. Got ${buffer.length}.`);
}
}
Expand Down Expand Up @@ -176,21 +170,10 @@ export class EthAddress {
}

/**
* Alias for toBuffer32.
* @returns A 32-byte Buffer containing the padded Ethereum address.
* Returns a 20-byte buffer representation of the Ethereum address.
* @returns A 20-byte Buffer containing the Ethereum address.
*/
public toBuffer() {
return this.toBuffer32();
}

/**
* Returns the internal Buffer representation of the Ethereum address.
* This method is useful when working with raw binary data or when
* integrating with other modules that require a Buffer as input.
*
* @returns A Buffer instance containing the 20-byte Ethereum address.
*/
public toBuffer20() {
return this.buffer;
}

Expand All @@ -201,6 +184,7 @@ export class EthAddress {
*
* @returns A 32-byte Buffer containing the padded Ethereum address.
*/
// TODO(#3938): nuke this
public toBuffer32() {
const buffer = Buffer.alloc(32);
this.buffer.copy(buffer, 12);
Expand All @@ -225,14 +209,19 @@ export class EthAddress {
return new EthAddress(fr.toBuffer().slice(-EthAddress.SIZE_IN_BYTES));
}

static fromFields(fields: Fr[] | FieldReader) {
const reader = FieldReader.asReader(fields);
return EthAddress.fromField(reader.readField());
}

/**
* Deserializes from a buffer or reader, corresponding to a write in cpp.
* @param buffer - Buffer to read from.
* @returns The EthAddress.
*/
static fromBuffer(buffer: Buffer | BufferReader): EthAddress {
const reader = BufferReader.asReader(buffer);
return new EthAddress(reader.readBytes(32));
return new EthAddress(reader.readBytes(EthAddress.SIZE_IN_BYTES));
}

/**
Expand Down
29 changes: 0 additions & 29 deletions yarn-project/foundation/src/serialize/serialize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,6 @@ export type Bufferable =
| Buffer
| number
| string
| {
/**
* Serialize to a buffer of 32 bytes.
*/
toBuffer32: () => Buffer;
}
| {
/**
* Serialize to a buffer.
Expand All @@ -135,27 +129,6 @@ export type Fieldeable =
}
| Fieldeable[];

/**
* Checks whether an object implements the toBuffer32 method.
* @param obj - The object to check.
* @returns Whether the object implements the toBuffer32 method.
*/
function isSerializableToBuffer32(obj: object): obj is {
/**
* Signature of the target serialization function.
*/
toBuffer32: () => Buffer;
} {
return !!(
obj as {
/**
* Signature of the target serialization function.
*/
toBuffer32: () => Buffer;
}
).toBuffer32;
}

/**
* Serializes a list of objects contiguously.
* @param objs - Objects to serialize.
Expand All @@ -176,8 +149,6 @@ export function serializeToBufferArray(...objs: Bufferable[]): Buffer[] {
} else if (typeof obj === 'string') {
ret.push(numToUInt32BE(obj.length));
ret.push(Buffer.from(obj));
} else if (isSerializableToBuffer32(obj)) {
ret.push(obj.toBuffer32());
} else {
ret.push(obj.toBuffer());
}
Expand Down
Loading

0 comments on commit 153989f

Please sign in to comment.