Skip to content

refactor!: note interfaces#12106

Merged
benesjan merged 7 commits intomasterfrom
02-19-refactor_note_interfaces
Feb 26, 2025
Merged

refactor!: note interfaces#12106
benesjan merged 7 commits intomasterfrom
02-19-refactor_note_interfaces

Conversation

@benesjan
Copy link
Contributor

@benesjan benesjan commented Feb 19, 2025

Fixes #12012

Implements a solution described in this comment

@benesjan benesjan changed the title refactor: note interfaces refactor!: note interfaces Feb 19, 2025
Copy link
Contributor Author

benesjan commented Feb 19, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 87bf0e8 to 958aca1 Compare February 19, 2025 15:23
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 21d1e1c to a004487 Compare February 19, 2025 16:17
@benesjan benesjan marked this pull request as ready for review February 19, 2025 16:20

Since the `entrypoint` interface is not enshrined, there is nothing that differentiates an account contract from an application one in the protocol. This means that a transaction can be initiated in any contract. This allows implementing functions that do not need to be called by any particular user and are just intended to advance the state of a contract.

As an example, we can think of a lottery contract, where at some point a prize needs to be paid out to its winners. This `pay` action does not require authentication and does not need to be executed by any user in particular, so anyone could submit a transaction that defines the lottery contract itself as `origin` and `pay` as entrypoint function. For an example of this behavior see our [non_contract_account test](https://github.com/AztecProtocol/aztec-packages/blob/master/yarn-project/end-to-end/src/e2e_non_contract_account.test.ts) and the [SignerLess wallet](https://github.com/AztecProtocol/aztec-packages/blob/master/yarn-project/aztec.js/src/wallet/signerless_wallet.ts) implementation.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nuke the test link because I nuked the test because it was some stale thing that made no sense.

@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch 2 times, most recently from ea68afa to 15d71a3 Compare February 20, 2025 08:49
@benesjan benesjan marked this pull request as draft February 20, 2025 09:01
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 15d71a3 to c9a156d Compare February 20, 2025 11:43
@@ -1,53 +0,0 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this to implementing a note and largely rewrote that as it was very stale.

@@ -25,8 +25,11 @@ The 3 test accounts deployed in the sandbox are pre-funded with 10 ^ 22 fee juic
In addition to the native fee juice, users can pay the transaction fees using tokens that have a corresponding FPC contract. The sandbox now includes `BananaCoin` and `BananaFPC`. Users can use a funded test account to mint banana coin for a new account. The new account can then start sending transactions and pay fees with banana coin.

```typescript
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just applied formatting here.

+ );
```

### [Aztec.nr] Changes to `NoteInterface`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is where real changes begin.

use crate::macros::utils::{
add_to_field_slice, AsStrQuote, compute_fn_selector, get_trait_impl_method, is_fn_private,
is_fn_view,
add_to_field_slice, AsStrQuote, compute_fn_selector, is_fn_private, is_fn_view,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just contained an unused import

) -> NoteGetterOptions<ValueNote, N, Field, Field>
where
ValueNote: Packable<N>,
{
Copy link
Contributor Author

@benesjan benesjan Feb 20, 2025

Choose a reason for hiding this comment

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

Sneaked this in (Nico 😝) - we relied on VALUE_NOTE_LEN being exported which is an anti-pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is interesting.

// TODO(#12008): Remove the need for the manual import of `Packable` trait here. This is a bug in macros.
use aztec::protocol_types::traits::Packable;

pub(crate) global VALUE_NOTE_LEN: u32 = 3; // 3 plus a header.
Copy link
Contributor Author

@benesjan benesjan Feb 20, 2025

Choose a reason for hiding this comment

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

Sneaked this in (Nico 😝) - we relied on VALUE_NOTE_LEN being exported which is an anti-pattern. On top of that now it could cause a problem if the derived Packable impl. did not match it.


#[private]
// Adapted from TokenContract#redeem_shield but without an initcheck so it can be run in simulator/src/client/private_execution.test.ts
fn consume_note_from_secret(secret: Field) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just used in one test which made zero sense so I just nuked it all.

@@ -1,54 +0,0 @@
import { ExtendedNote, Fr, type Logger, Note, type Wallet } from '@aztec/aztec.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was just weird - it contains just one test case which looks useless at this point and the test case has nothing to do with non_contract_accounts. So I just nuked it all.

});
});

it('Should be able to consume a dummy public to private message', async () => {
Copy link
Contributor Author

@benesjan benesjan Feb 20, 2025

Choose a reason for hiding this comment

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

This test seems like a non-sense. It does nothing with messaging!

This test case was first created 20 months ago and I think it has been updated like 426 times in a negligent way that it became something completely different. So I nuked it

@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 79e5879 to d724762 Compare February 20, 2025 12:27
@benesjan benesjan marked this pull request as ready for review February 20, 2025 12:27
@@ -1,18 +1,21 @@
use crate::types::card_note::{CARD_NOTE_LEN, CardNote};
use crate::types::card_note::CardNote;
Copy link
Contributor Author

@benesjan benesjan Feb 20, 2025

Choose a reason for hiding this comment

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

Sneaked changes in this file in (Nico 😝) - we relied on CARD_NOTE_LEN being exported which is an anti-pattern.

// docs:start:address_note_struct
// Stores an address
#[note]
#[derive(Eq, Serialize)]
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 Serialize trait impl. was unused here so I nuked it.

@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 7f791ad to 7bcada0 Compare February 20, 2025 14:32
@benesjan benesjan requested a review from charlielye as a code owner February 20, 2025 14:32
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from 3f3f111 to cf57f73 Compare February 21, 2025 15:35
@nventuro nventuro removed the request for review from charlielye February 24, 2025 16:22
) -> NoteGetterOptions<ValueNote, N, Field, Field>
where
ValueNote: Packable<N>,
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is interesting.

@benesjan benesjan marked this pull request as draft February 24, 2025 18:02
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch 3 times, most recently from 0914eaa to 4690b3d Compare February 25, 2025 13:18
@benesjan benesjan marked this pull request as ready for review February 25, 2025 13:28
@benesjan benesjan requested a review from nventuro February 25, 2025 14:03
) -> [u8; N * 32 + 64]
where
Note: NoteInterface + Packable<N>,
Note: NoteType + Packable<N>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Been thinking for a while now that this should be NOTE, not Note. Note looks like a type, and we always use caps for generics in other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. below Note::get_id() looks quite confusing - as if Note was a trait, which also sounds quite reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Created an issue and I will tackle it once we get our note-related PRs merged.

@benesjan benesjan marked this pull request as draft February 25, 2025 19:01
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from cea2e92 to e1c19b9 Compare February 25, 2025 19:10
@benesjan benesjan marked this pull request as ready for review February 25, 2025 19:13
@benesjan benesjan enabled auto-merge (squash) February 25, 2025 19:14
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch 2 times, most recently from 62a0c50 to f6e1600 Compare February 25, 2025 20:57
@benesjan benesjan removed the e2e-all label Feb 25, 2025
@benesjan benesjan force-pushed the 02-19-refactor_note_interfaces branch from f6e1600 to 5b28f84 Compare February 25, 2025 21:47
@benesjan benesjan merged commit 63081a4 into master Feb 26, 2025
10 checks passed
@benesjan benesjan deleted the 02-19-refactor_note_interfaces branch February 26, 2025 00:18
TomAFrench added a commit that referenced this pull request Feb 26, 2025
* master: (92 commits)
  chore: Log prover publisher address on creation (#12267)
  feat: https for bootnode in devnet (#12161)
  feat(avm): support shifts in lookups (#12280)
  feat(docs): Add flamegraph tool to counter contract tutorial (#12202)
  feat(spartan): 192 node 1 tps - additional validator service (#12238)
  feat(avm): class id derivation (#12263)
  docs: Fees doc snippets and code snippets (#12229)
  refactor: proving cost in fee header (#12048)
  fix: prometheus scrapes itself in the cluster (#12277)
  feat: metrics (#12256)
  chore: cleanup stdlib internal imports (#12274)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  fix: Enforce no import side effects (#12268)
  refactor!: note interfaces (#12106)
  yolo undenoise tests
  feat: new transcript functionality to hash elements without including in proof (#12233)
  chore: remove gcloud metrics (#12265)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom implementation of NoteInterface::get_note_type_id needs to return the same id as generated by macros

3 participants