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: leak escrow keys #54

Closed
wants to merge 15 commits into from
Closed

feat: leak escrow keys #54

wants to merge 15 commits into from

Conversation

xorsal
Copy link
Collaborator

@xorsal xorsal commented Mar 11, 2025

I'm re-enabling the call to Escrow::leak_keys and have modified the E2E tests so that Bob can claim the escrow's tokens without cheating (i.e., without manually passing it), but instead by retrieving the secret from the private log emitted by the contract.

The nullification_secret in the EscrowKeys event is not currently used, but I'm keeping it for now to explore potential future use cases

@xorsal xorsal requested a review from ilpepepig March 14, 2025 14:53
@ilpepepig
Copy link
Collaborator

This is redundant with get_notes() which seems to constrain the notes according to the options provided.

assert(unwrapped_note.escrow == escrow, "Invalid escrow address");

Same with these assertions

assert(note.escrow == escrow, "Invalid escrow address");

assert(note.escrow == escrow, "Invalid escrow address");

@ilpepepig
Copy link
Collaborator

For consistency with claim() let's use note.sender instead of caller?

Escrow::at(escrow).withdraw(token, amount, caller).call(&mut context);

Copy link
Collaborator

@ilpepepig ilpepepig left a comment

Choose a reason for hiding this comment

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

self.escrow.to_field() can be removed as it is redundant with the content of the note

[note_hash_for_nullify, self.escrow.to_field()],

Also, some dependencies imported into the escrow and clawback escrow contracts, and tests are not used.

The nullification_secret in the EscrowKeys event is not currently used, but I'm keeping it for now to explore potential future use cases

Regarding this, if nullification_secret can be derived from the escrow secret, then it is redundant to emit it.

@ilpepepig
Copy link
Collaborator

In the escrow and clawback escrow e2e tests, beforeAll() is implemented slightly different. It would be nice to make them consistent.

Also, is this still an issue?

// TODO: why do I need to register Alice's account?

xorsal added 6 commits March 14, 2025 16:26
chore: add missing colon
o I remove the additional checks,

Flamegraph shows a difference of one opcode, but gate-couting remains
the same in both functions.

without asserts:
Opcode count: 1992, Total gates by opcodes: 10564, Circuit size: 10621
with asserts:
Opcode count: 1993, Total gates by opcodes: 10564, Circuit size: 10621
@xorsal
Copy link
Collaborator Author

xorsal commented Mar 17, 2025

Closing this, migrated to defi-wonderland/aztec-extensions

@xorsal xorsal closed this Mar 17, 2025
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.

2 participants