-
Notifications
You must be signed in to change notification settings - Fork 306
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
feat: internal keyword + lending contract and tests #978
Changes from 16 commits
673284a
c4b6264
684e14b
46d6e9e
0f62615
bc58383
876df6b
7aa3e02
6191d5e
b3f6e88
cfe0493
1245143
3689ab4
c6a2b12
9b22e07
2c6102d
25cf778
746b74e
fac6b3c
b41a7dc
f123135
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
#include "aztec3/utils/types/convert.hpp" | ||
#include "aztec3/utils/types/native_types.hpp" | ||
|
||
#include "barretenberg/common/serialize.hpp" | ||
|
||
namespace aztec3::circuits::abis { | ||
|
||
using aztec3::utils::types::CircuitTypes; | ||
|
@@ -32,14 +34,15 @@ template <typename NCT> struct FunctionLeafPreimage { | |
using uint32 = typename NCT::uint32; | ||
|
||
uint32 function_selector = 0; | ||
boolean is_internal = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This being almost the same as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly, 3 are overlapping. But the function data is accesible inside noir as well, so maybe the leaf should include the function data: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the sounds of that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think perhaps the difference between
Some of this is faint memories, so I could be wrong, but I believe their separation was intentional. Re storing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, it can stay as is! |
||
boolean is_private = false; | ||
fr vk_hash = 0; | ||
fr acir_hash = 0; | ||
|
||
boolean operator==(FunctionLeafPreimage<NCT> const& other) const | ||
{ | ||
return function_selector == other.function_selector && is_private == other.is_private && | ||
vk_hash == other.vk_hash && acir_hash == other.acir_hash; | ||
return function_selector == other.function_selector && is_internal == other.is_internal && | ||
is_private == other.is_private && vk_hash == other.vk_hash && acir_hash == other.acir_hash; | ||
}; | ||
|
||
template <typename Builder> FunctionLeafPreimage<CircuitTypes<Builder>> to_circuit_type(Builder& builder) const | ||
|
@@ -50,10 +53,7 @@ template <typename NCT> struct FunctionLeafPreimage { | |
auto to_ct = [&](auto& e) { return aztec3::utils::types::to_ct(builder, e); }; | ||
|
||
FunctionLeafPreimage<CircuitTypes<Builder>> preimage = { | ||
to_ct(function_selector), | ||
to_ct(is_private), | ||
to_ct(vk_hash), | ||
to_ct(acir_hash), | ||
to_ct(function_selector), to_ct(is_internal), to_ct(is_private), to_ct(vk_hash), to_ct(acir_hash), | ||
}; | ||
|
||
return preimage; | ||
|
@@ -65,10 +65,7 @@ template <typename NCT> struct FunctionLeafPreimage { | |
auto to_nt = [&](auto& e) { return aztec3::utils::types::to_nt<Builder>(e); }; | ||
|
||
FunctionLeafPreimage<NativeTypes> preimage = { | ||
to_nt(function_selector), | ||
to_nt(is_private), | ||
to_nt(vk_hash), | ||
to_nt(acir_hash), | ||
to_nt(function_selector), to_nt(is_internal), to_nt(is_private), to_nt(vk_hash), to_nt(acir_hash), | ||
}; | ||
|
||
return preimage; | ||
|
@@ -79,6 +76,7 @@ template <typename NCT> struct FunctionLeafPreimage { | |
static_assert(!(std::is_same<NativeTypes, NCT>::value)); | ||
|
||
function_selector.set_public(); | ||
fr(is_internal).set_public(); | ||
fr(is_private).set_public(); | ||
vk_hash.set_public(); | ||
acir_hash.set_public(); | ||
|
@@ -87,10 +85,7 @@ template <typename NCT> struct FunctionLeafPreimage { | |
fr hash() const | ||
{ | ||
std::vector<fr> const inputs = { | ||
function_selector, | ||
fr(is_private), | ||
vk_hash, | ||
acir_hash, | ||
function_selector, fr(is_internal), fr(is_private), vk_hash, acir_hash, | ||
}; | ||
return NCT::compress(inputs, GeneratorIndex::FUNCTION_LEAF); | ||
} | ||
|
@@ -101,6 +96,7 @@ template <typename NCT> void read(uint8_t const*& it, FunctionLeafPreimage<NCT>& | |
using serialize::read; | ||
|
||
read(it, preimage.function_selector); | ||
read(it, preimage.is_internal); | ||
read(it, preimage.is_private); | ||
read(it, preimage.vk_hash); | ||
read(it, preimage.acir_hash); | ||
|
@@ -111,6 +107,7 @@ template <typename NCT> void write(std::vector<uint8_t>& buf, FunctionLeafPreima | |
using serialize::write; | ||
|
||
write(buf, preimage.function_selector); | ||
write(buf, preimage.is_internal); | ||
write(buf, preimage.is_private); | ||
write(buf, preimage.vk_hash); | ||
write(buf, preimage.acir_hash); | ||
|
@@ -119,6 +116,7 @@ template <typename NCT> void write(std::vector<uint8_t>& buf, FunctionLeafPreima | |
template <typename NCT> std::ostream& operator<<(std::ostream& os, FunctionLeafPreimage<NCT> const& preimage) | ||
{ | ||
return os << "function_selector: " << preimage.function_selector << "\n" | ||
<< "is_internal: " << preimage.is_internal << "\n" | ||
<< "is_private: " << preimage.is_private << "\n" | ||
<< "vk_hash: " << preimage.vk_hash << "\n" | ||
<< "acir_hash: " << preimage.acir_hash << "\n"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,15 @@ void common_validate_inputs(DummyBuilder& builder, KernelInput const& public_ker | |
builder.do_assert(public_kernel_inputs.public_call.bytecode_hash != 0, | ||
"Bytecode hash must be non-zero", | ||
CircuitErrorCode::PUBLIC_KERNEL__BYTECODE_HASH_INVALID); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a cpp test testing this code path, similarly for the other assert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, will do. It's tested in the TS as well. |
||
if (this_call_stack_item.function_data.is_internal) { | ||
auto const target = this_call_stack_item.contract_address; | ||
auto const sender = this_call_stack_item.public_inputs.call_context.msg_sender; | ||
|
||
builder.do_assert(target == sender, | ||
"call is internal, but msg_sender is not self", | ||
CircuitErrorCode::PUBLIC_KERNEL__IS_INTERNAL_BUT_NOT_SELF_CALL); | ||
} | ||
} | ||
|
||
template <typename KernelInput, typename Builder> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,7 +99,7 @@ describe('Private Execution test suite', () => { | |
const txRequest = TxExecutionRequest.from({ | ||
origin, | ||
argsHash: packedArguments.hash, | ||
functionData: new FunctionData(Buffer.alloc(4), true, isConstructor), | ||
functionData: new FunctionData(Buffer.alloc(4), false, true, isConstructor), | ||
txContext: TxContext.from({ ...txContextFields, ...txContext }), | ||
packedArguments: [packedArguments], | ||
}); | ||
|
@@ -388,7 +388,7 @@ describe('Private Execution test suite', () => { | |
expect(result.callStackItem.publicInputs.returnValues[0]).toEqual(new Fr(initialValue + privateIncrement)); | ||
}); | ||
|
||
it('parent should call child', async () => { | ||
it.only('parent should call child', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bonk There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. think this got included by mistake |
||
const childAbi = ChildContractAbi.functions.find(f => f.name === 'value')!; | ||
const parentAbi = ParentContractAbi.functions.find(f => f.name === 'entryPoint')!; | ||
const parentAddress = AztecAddress.random(); | ||
|
@@ -500,14 +500,15 @@ describe('Private Execution test suite', () => { | |
}); | ||
|
||
describe('enqueued calls', () => { | ||
it('parent should enqueue call to child', async () => { | ||
it.each([false, true])('parent should enqueue call to child', async isInternal => { | ||
const parentAbi = ParentContractAbi.functions.find(f => f.name === 'enqueueCallToChild')!; | ||
const childAddress = AztecAddress.random(); | ||
const childPortalContractAddress = EthAddress.random(); | ||
const childSelector = Buffer.alloc(4, 1); // should match the call | ||
const parentAddress = AztecAddress.random(); | ||
|
||
oracle.getPortalContractAddress.mockImplementation(() => Promise.resolve(childPortalContractAddress)); | ||
oracle.getFunctionABI.mockImplementation(() => Promise.resolve({ ...ChildContractAbi.functions[0], isInternal })); | ||
|
||
const args = [Fr.fromBuffer(childAddress.toBuffer()), Fr.fromBuffer(childSelector), 42n]; | ||
const result = await runSimulator({ | ||
|
@@ -519,7 +520,7 @@ describe('Private Execution test suite', () => { | |
|
||
const publicCallRequest = PublicCallRequest.from({ | ||
contractAddress: childAddress, | ||
functionData: new FunctionData(childSelector, false, false), | ||
functionData: new FunctionData(childSelector, isInternal, false, false), | ||
args: [new Fr(42n)], | ||
callContext: CallContext.from({ | ||
msgSender: parentAddress, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,7 +260,7 @@ export class PrivateFunctionExecution { | |
curve: Curve, | ||
) { | ||
const targetAbi = await this.context.db.getFunctionABI(targetContractAddress, targetFunctionSelector); | ||
const targetFunctionData = new FunctionData(targetFunctionSelector, true, false); | ||
const targetFunctionData = new FunctionData(targetFunctionSelector, targetAbi.isInternal, true, false); | ||
const derivedCallContext = await this.deriveCallContext(callerContext, targetContractAddress, false, false); | ||
const context = this.context.extend(); | ||
|
||
|
@@ -293,11 +293,12 @@ export class PrivateFunctionExecution { | |
targetArgs: Fr[], | ||
callerContext: CallContext, | ||
): Promise<PublicCallRequest> { | ||
const targetAbi = await this.context.db.getFunctionABI(targetContractAddress, targetFunctionSelector); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nicely done |
||
const derivedCallContext = await this.deriveCallContext(callerContext, targetContractAddress, false, false); | ||
return PublicCallRequest.from({ | ||
args: targetArgs, | ||
callContext: derivedCallContext, | ||
functionData: new FunctionData(targetFunctionSelector, false, false), | ||
functionData: new FunctionData(targetFunctionSelector, targetAbi.isInternal, false, false), | ||
contractAddress: targetContractAddress, | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,7 +182,10 @@ export class PublicExecutor { | |
globalVariables: GlobalVariables, | ||
) { | ||
const portalAddress = (await this.contractsDb.getPortalContractAddress(targetContractAddress)) ?? EthAddress.ZERO; | ||
const functionData = new FunctionData(targetFunctionSelector, false, false); | ||
// @todo @lherskind - Need to make this data accessible | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there an issue for this todo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it is #1200 mentioned in the pr here. Will add the link in there as well. |
||
//const abi = await this.contractsDb.getFunctionABI(targetContractAddress, targetFunctionSelector); | ||
const isInternal = false; | ||
const functionData = new FunctionData(targetFunctionSelector, isInternal, false, false); | ||
|
||
const callContext = CallContext.from({ | ||
msgSender: callerContext.storageContractAddress, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this not neede before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not needed actually, but just leftovers from some fiddling in the middle.