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

feat: Token standard #2069

Merged
merged 29 commits into from
Sep 13, 2023
Merged

feat: Token standard #2069

merged 29 commits into from
Sep 13, 2023

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Sep 6, 2023

Fixes #1743.

See #2199 for extensions that is required with generators etc to not collide with payloads.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@LHerskind LHerskind added the S-blocked Status: Blocked label Sep 7, 2023
@iAmMichaelConnor
Copy link
Contributor

We decided to get rid of storage.nr files and bring Storage declarations into the main.nr file - Leila updated the rest of the contract examples today

@LHerskind LHerskind force-pushed the lh/token-standard branch 2 times, most recently from 9c06dd5 to de9ec7f Compare September 8, 2023 08:56
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Love it

if (from.address != context.msg_sender()) {
let selector = compute_selector("transfer_public((Field),(Field),Field,Field)");
let message_field = compute_message_hash([context.msg_sender(), selector, from.address, to.address, amount, nonce]);
assert(AccountContract::at(from.address).is_valid(Option::none(), Option::some(context), message_field) == 0xe86ab4ff, "invalid call");
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI I had added a helper for this here: https://github.com/AztecProtocol/aztec-packages/blob/master/yarn-project/noir-libs/noir-aztec/src/auth.nr. Maybe we can include it in your account contract interface and remove it as a separate util?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, seems nice. Will update to use that one instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewed, and think I prefer the combined is_valid that handles both the private and public flows with same interface more 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, but I think there's value in a helper that removes the need for hardcoding the 0xe86ab4ff selector in your app logic. Maybe add that check to the account interface?

}

#[aztec(private)]
fn set_is_valid_storage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We haven't even launched and have already found a use case for transient storage

Comment on lines 150 to 144
#[aztec(public)]
fn mint_priv(
amount: Field,
secret_hash: Field,
) -> Field {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that the only thing that prevents us from having a private mint method is being able to access the list of minters from private-land, and for that we'd need the slow-updates tree, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but even if in private I would want it to do a public call to update the total_supply such that it is up to date. But yes you could mint directly into a specific user if we got slow-updates tree.

Comment on lines 90 to 87
let storage = Storage::init(Option::none(), Option::some(&mut context));
let value = storage.approved_action.at(message_hash).read();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Going back to transient storage: I believe that if we write zero again to this slot when "consuming" the approval, then the public kernel circuit should "squash" those updates and not impact the public data tree. I may be mistaken though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, ideally, I think I have a note somewhere about sandwiching your own tx with a set and unset such that it is just a transient "approval".

assert(AccountContract::at(from.address).is_valid(Option::none(), Option::some(context), message_field) == 0xe86ab4ff, "invalid call");
context.push_new_nullifier(message_field, 0);
} else {
assert(nonce == 0, "invalid nonce");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because I did not like a value that could be whatever when not used, so making it 0 made it pretty clear as well if doing a direct transfer or the transfer from.

Copy link
Contributor

@rahul-kothari rahul-kothari left a comment

Choose a reason for hiding this comment

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

Looks great - one major nit - a function that should be internal is not

}

createTxExecutionRequest(_executions: FunctionCall[], _opts?: CreateTxRequestOpts): Promise<TxExecutionRequest> {
throw new Error('Method not implemented.');
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a TODO with an issue to either completely remove or implement it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface for Entrypoints include it, but it is error-prone if used with the Auth wallet.

let witness = get_auth_witness(message_hash);
assert(recover_address(message_hash, witness) == address);
true
Copy link
Contributor

@rahul-kothari rahul-kothari Sep 11, 2023

Choose a reason for hiding this comment

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

I see internal keyword is missing for this function? Is this intentional? If so shouldn't this too return fn selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should not be callable since it is not using #[aztec(private)] nr #[aztec(public)]. But can see that the abi is read as if it is. Will try executing it to see what actually happens.

// The designated caller is ALWAYS used here, and not based on a flag as cross-chain.
// message hash = H([caller, selector, , ...args])
// To be read as `caller` calls function defined by `selector` with `args`
// Including a nonce in the message hash ensures that the message can only be used once.
Copy link
Contributor

Choose a reason for hiding this comment

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

<3 this is clean!!

}

#[aztec(public)]
fn mint_priv(
Copy link
Contributor

Choose a reason for hiding this comment

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

public function but called mint_priv aaaah. I am out of ideas on a better name though :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. The name gets a little funky.

} else if public_context.is_some() {
public_context.unwrap().call_public_function(
self.address,
0xf3661153,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here that this calls is_valid_public which still returns 0xe86ab4ff (same as is_valid from private land).
For some reason, I expected this to return 0xf3661153 because I see that this is the selector lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ye ok, that would probably have made somewhat sense to do. Just used the same selector return as the only reason it got the public one was that we can't have functions that have both a public and private flow hehe

use dep::aztec::constants_gen::GENERATOR_INDEX__SIGNATURE_PAYLOAD;

fn compute_message_hash<N>(args: [Field; N]) -> Field {
// @todo @lherskind We should probably use a separate generator for this,
Copy link
Contributor

Choose a reason for hiding this comment

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

does this require a new issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should become part of fixes to #2199

@@ -141,6 +141,14 @@ export class ViemTxSender implements L1PublisherTxSender {
`0x${extendedContractData.bytecode.toString('hex')}`,
] as const;

if (extendedContractData.bytecode.length > 131072) {
this.log(
`Bytecode is too large ethereum transactions: ${extendedContractData.bytecode.length} ${
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean to say how many eth transactions it would take to deploy the contract? Maybe make the message clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Were partly for debugging and making it clear when large code is generated. But will make it more clear.

let selector = compute_selector("transfer_public((Field),(Field),Field,Field)");
let message_field = compute_message_hash([context.msg_sender(), selector, from.address, to.address, amount, nonce]);
assert(AccountContract::at(from.address).is_valid(Option::none(), Option::some(context), message_field) == 0xe86ab4ff, "invalid call");
context.push_new_nullifier(message_field, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a comment here on why adding the nullifier is very very important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should become part of fixes to #2199

Copy link
Contributor

@rahul-kothari rahul-kothari left a comment

Choose a reason for hiding this comment

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

legend!

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Some minor things id like changed, also seem to have spotted some aztec noir bugs :(

const enqueuedRequest = await this.enqueuePublicFunctionCall(
frToAztecAddress(fromACVMField(acvmContractAddress)),
selector,
Copy link
Member

Choose a reason for hiding this comment

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

This takes in the targetContractAddress, why is it called selector?

In executor this name is used again but with different fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I copied it a round because the 32byte strings tilted me on some error codes. Will fix it.

@@ -141,6 +141,10 @@ export class ViemTxSender implements L1PublisherTxSender {
`0x${extendedContractData.bytecode.toString('hex')}`,
] as const;

const codeSize = extendedContractData.bytecode.length;
const blobSize = 31 * 4096;
Copy link
Member

Choose a reason for hiding this comment

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

I think blobSize should be part of constants.ts

const [block] = await this.blockBuilder.buildL2Block(globalVariables, allTxs, newL1ToL2Messages);
return block;
} catch (err) {
const hashes = txs.map(tx => tx.hash);
Copy link
Member

Choose a reason for hiding this comment

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

this is dropping all transactions, even if only one of them is offending?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we were only building blocks of 1 tx for the e2e, so should not be an issue. But I'm looking at it now because It gave other issues for the race in p2p test.
Figured that the emission of same nullifier was not caught before the block building by the sequencer so it was unclear exactly what was offending, but adding a check earlier on.

storage.public_balances.at(owner.address).read()
}

// Below this point is the stuff of nightmares.
Copy link
Member

Choose a reason for hiding this comment

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

haha do we want this comment in our token contract?

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 have done worse 🤷 We need to fix the issue down there as well though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's reword this comment to explain some tangible TODOs.

}

#[aztec(public)]
fn mint_pub(
Copy link
Member

Choose a reason for hiding this comment

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

Lets use non abbreviated terms in the method names

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Some minor things id like changed, also seem to have spotted some aztec noir bugs :(

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Gave it a second pass, sorry ive a bit more

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

lully stuff my man

@Maddiaa0 Maddiaa0 enabled auto-merge (squash) September 13, 2023 10:55
@Maddiaa0 Maddiaa0 merged commit 5e8fbf2 into master Sep 13, 2023
88 checks passed
@Maddiaa0 Maddiaa0 deleted the lh/token-standard branch September 13, 2023 12:57
Copy link
Contributor

@iAmMichaelConnor iAmMichaelConnor left a comment

Choose a reason for hiding this comment

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

I've added some belated comments to the token contract :)

from: AztecAddress,
to: AztecAddress,
amount: Field,
nonce: Field,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this has been merged, but I'll add some Q's / comments, now I've found time to take a look :)
What's the nonce for? Could it be given a more descriptive name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nonce is used in the message to make it possible to emit the message as nullifier to avoid replays without wreaking same action later.

Copy link
Contributor

@iAmMichaelConnor iAmMichaelConnor Sep 15, 2023

Choose a reason for hiding this comment

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

message_nonce, perhaps?
authwit_nonce?
auth_message_nonce?

}

let amount = SafeU120::new(amount);
let from_balance = SafeU120::new(storage.public_balances.at(from.address).read()).sub(amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe do let mut public_balances = storage.public_balances; at the beginning of the function, to reduce storage.__ later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh. I kinda like it being very explicit that it is storage.


if (from.address != context.msg_sender()) {
let selector = compute_selector("burn_public((Field),Field,Field)");
let message_field = compute_message_hash([context.msg_sender(), context.this_address(), selector, from.address, amount, nonce]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe call it message_hash given the name of the function is compute_message_hash.

fn transfer(
from: AztecAddress,
to: AztecAddress,
amount: Field,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the amounts in all functions eventually be a safer type than Field, i.e. some kind of uint (once Noir is changed to prevent under/overflows?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually. But for now that is not safe.

Comment on lines +404 to +405
// Below this point is the stuff of nightmares.
// This should ideally not be required. What do we do if vastly different types of preimages?
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we'll likely need to define many 'sizes' of compute_note_hash_and_nullifier, there should be a way for the note processor to distinguish between different note types. The ciphertexts probably need to contain some identifier for the note type.

This should ideally not be required

I'm not sure I agree with this. Having the Noir Contract express how the private state database should be populated (as is done here) gives lots of control to developers. It took a lot of discussions to get to this pattern :')

storage.public_balances.at(owner.address).read()
}

// Below this point is the stuff of nightmares.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's reword this comment to explain some tangible TODOs.

Comment on lines +14 to +45
struct AztecAddress {
address: Field
}

impl AztecAddress {
fn new(address: Field) -> Self {
Self {
address
}
}

fn serialize(self: Self) -> [Field; 1] {
[self.address]
}

fn deserialize(fields: [Field; 1]) -> Self {
Self {
address: fields[0]
}
}
}

struct EthereumAddress {
address: Field
}

impl EthereumAddress {
fn new(address: Field) -> Self {
Self {
address
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

AztecAddress and EthereumAddress should be added to aztec-nr/ instead, since they're widely applicable types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pedersen_with_separator([secret], GENERATOR_INDEX__L1_TO_L2_MESSAGE_SECRET)[0]
}

fn knows_secret(self, secret: Field) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment to say that this is a custom method that's not enforced by the NoteInterface

PhilWindle pushed a commit that referenced this pull request Sep 14, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.7.1</summary>

##
[0.7.1](aztec-packages-v0.7.0...aztec-packages-v0.7.1)
(2023-09-14)


### Features

* Build system handles dynamic deps first class.
([#2283](#2283))
([f66077a](f66077a))
* Build_manifest default tweaks.
([#2287](#2287))
([c8a5cfb](c8a5cfb))
* **build:** Build multi-architecture docker images for aztec-sandbox
([#2305](#2305))
([8ee61b8](8ee61b8))
* Cli "unbox" command
([#2029](#2029))
([26ab88f](26ab88f))
* Creating an SMT verification module
([#1932](#1932))
([4642b61](4642b61))
* Token standard
([#2069](#2069))
([5e8fbf2](5e8fbf2))


### Bug Fixes

* Ensure_note_hash_exists
([#2256](#2256))
([271b060](271b060))
* Msgpack stack blowups on schema gen
([#2259](#2259))
([1afc566](1afc566))
* Noir bootstrap
([#2274](#2274))
([f85db49](f85db49))
* Workaround sequencer timeout
([#2269](#2269))
([9fc3f3d](9fc3f3d))


### Miscellaneous

* Bump nargo to 0.11.1-aztec.0
([#2298](#2298))
([8b76a12](8b76a12))
* **ci:** Mirror Aztec-nr
([#2270](#2270))
([c57f027](c57f027))
* **circuits:** Base rollup cbind msgpack
([#2263](#2263))
([0d4c707](0d4c707))
* **circuits:** Clean up of some superfluous header includes
([#2302](#2302))
([5e53345](5e53345))
* **circuits:** Removing assertMemberLength on Tuple objects
([#2296](#2296))
([0247b85](0247b85))
* Consolidate mirror repos on a nightly schedule
([#1994](#1994))
([1a586c4](1a586c4))
* **docs:** Rename to aztec.nr
([#1943](#1943))
([a91db48](a91db48))
* Move barretenberg to top of repo. Make circuits build off barretenberg
build.
([#2221](#2221))
([404ec34](404ec34))
* Replace native token in lending contract
([#2276](#2276))
([c46b3c8](c46b3c8))
* **subrepo:** Push aztec-nr, update default branches
([#2300](#2300))
([80c9b77](80c9b77))
* Updated `acvm_js`
([#2272](#2272))
([9f1a3a5](9f1a3a5))
</details>

<details><summary>barretenberg.js: 0.7.1</summary>

##
[0.7.1](barretenberg.js-v0.7.0...barretenberg.js-v0.7.1)
(2023-09-14)


### Miscellaneous

* Move barretenberg to top of repo. Make circuits build off barretenberg
build.
([#2221](#2221))
([404ec34](404ec34))
</details>

<details><summary>barretenberg: 0.7.1</summary>

##
[0.7.1](barretenberg-v0.7.0...barretenberg-v0.7.1)
(2023-09-14)


### Miscellaneous

* Move barretenberg to top of repo. Make circuits build off barretenberg
build.
([#2221](#2221))
([404ec34](404ec34))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Token interfaces and standard-ish
5 participants