feat: initial escrowing example#6632
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @LHerskind and the rest of your teammates on |
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
2 times, most recently
from
a6494b7 to
5a1438b
Compare
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
4 times, most recently
from
99a70a6 to
7df928c
Compare
| * Escrowable Token (partial implementation) | ||
| * | ||
| * The following is a partial implementation of an escrowable token. | ||
| * Partial in the sense that it do not (yet) support all the features of a full token, but just |
There was a problem hiding this comment.
| * Partial in the sense that it do not (yet) support all the features of a full token, but just | |
| * Partial in the sense that it does not (yet) support all the features of a full token, but just |
| * Escrowable here being that the "owner" of a "balance" might be different from the actor | ||
| * who is actually able to use the funds. | ||
| * | ||
| * In public state, example on this is uniswap most other contracts. The contract escrow funds |
There was a problem hiding this comment.
Sentence does not make sense
| * it defines the rules for how it is spend (still following the tokens own rules). | ||
| * | ||
| * This is all good and dandy, but when moving to private state, this becomes significantly more tricky. | ||
| * Recall in private you have both an "owner" as specified by the business logic, but also a actor |
There was a problem hiding this comment.
| * Recall in private you have both an "owner" as specified by the business logic, but also a actor | |
| * Recall in private you have both an "owner" as specified by the business logic, but also an actor |
| * However, if escrowing, this might no longer be the case. | ||
| * | ||
| * An example would be a `vault` such as the `SimpleEscrow` contract. | ||
| * The contract "owns" the tokens, but it don't have any nullifier keys itself, and rely on `alice` to provide them. |
There was a problem hiding this comment.
| * The contract "owns" the tokens, but it don't have any nullifier keys itself, and rely on `alice` to provide them. | |
| * The contract "owns" the tokens, but it doesn't have any nullifier keys itself, and relies on `alice` to provide them. |
| * be double spent, we need to support the case where `vault` is the `owner` but `alice`s keys are used for `npk_m_hash` | ||
| * | ||
| * Furthermore, to support the `incoming` and `outgoing` logs, we need to provie values for these. Note that | ||
| * want to use the address that maps to `npk_m_hash` for the `ivpk` as well, since we want to ensure that the |
There was a problem hiding this comment.
| * want to use the address that maps to `npk_m_hash` for the `ivpk` as well, since we want to ensure that the | |
| * we want to use the address that maps to `npk_m_hash` for the `ivpk` as well, since we want to ensure that the |
| * | ||
| * Furthermore, to support the `incoming` and `outgoing` logs, we need to provie values for these. Note that | ||
| * want to use the address that maps to `npk_m_hash` for the `ivpk` as well, since we want to ensure that the | ||
| * actor that can nullify the note, will also learn about the notes existence - otherwise it don't matter much. |
There was a problem hiding this comment.
| * actor that can nullify the note, will also learn about the notes existence - otherwise it don't matter much. | |
| * actor that can nullify the note, will also learn about the notes existence - otherwise it doesn't matter much. |
| * want to use the address that maps to `npk_m_hash` for the `ivpk` as well, since we want to ensure that the | ||
| * actor that can nullify the note, will also learn about the notes existence - otherwise it don't matter much. | ||
| * | ||
| * The address mapping to `ovpk` is a bit more tricky, since it don't necessarily match any of the other addresses. |
There was a problem hiding this comment.
| * The address mapping to `ovpk` is a bit more tricky, since it don't necessarily match any of the other addresses. | |
| * The address mapping to `ovpk` is a bit more tricky, since it doesn't necessarily match any of the other addresses. |
| * In most "direct" user to user transfers, it will belong to the `msg_sender`, but for escrowing, it might be someone | ||
| * else. It needs to be provided. | ||
| * | ||
| * - `incoming_viewer_and_nullifier` to refer to the address that is used to fetch `ivpk` and `npk` related values |
There was a problem hiding this comment.
I see that you don't like our nullificator friend
He will remember
| fn mint_private( | ||
| to: AztecAddress, | ||
| amount: Field, | ||
| incoming_viewer_and_nullifier: AztecAddressOption, |
There was a problem hiding this comment.
Great idea using option here 👍
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
from
7df928c to
fab4d38
Compare
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
2 times, most recently
from
293aebe to
0c04aaa
Compare
Docs PreviewHey there! 👋 You can check your preview at https://665f23d0839c0f1860d222da--aztec-docs-dev.netlify.app |
|
|
||
| let amount = U128::from_integer(amount); | ||
|
|
||
| storage.balances.sub( |
There was a problem hiding this comment.
imagine a staking contract.
When user stakes there: tokenNote should still belong to user. But balance should be increased of staking contract
i.e. balances.at(staking_contract) += amount
SAID ANOTHER WAY: balance[staking_contract] = TokenNote<owner = user. amount = 100>
Later, when a user unstakes:
the staking contract should reduce its balance. Crucuially though, get_notes should be called on the user and not the staking contract since the note.owner = user.
So there should be a from_nullifioor. My hypothesis is there is never a case when from_nullifioor != outgoing_viewoor so you should get_notes() from that party and not from
| ) where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote { | ||
| // docs:start:get_notes | ||
| let options = NoteGetterOptions::with_filter(filter_notes_min_sum, subtrahend); | ||
| let maybe_notes = self.map.at(owner).get_notes(options); |
There was a problem hiding this comment.
As discussed A potential issue not with this contract but PXE i guess:
Assume owner (person with balance) is different to address that gives nullifier key. E..g in escrow or staking contract:
balance[staking_contract] = [ TokenNote< { rahul, amount = 100}, TokenNote {lasse, amount = 50} ]
i.e. both rahul and lasse stake some tokens [the balance of staking contract increases (150) but lasse and rahul still own their note.
Later, when a user unstakes, the staking contract should reduce its balance. Crucuially though, get_notes should filter for notes with the right user.
Currently if whoever is calling the tx somehow has both Lasse and Rahul's notes, both get fetched and there is a non-deterministic outcome i.e. sometimes lasse's notes could be nullified even though I wanted to unstake rahul's notes.
Ideally the pxe can just filter based on the account address in the note and only return those notes
|
https://link.excalidraw.com/l/918xh05mbiS/AIKmHUXcmiH For those trying to understand what's happening with all these comments and scenarious |
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
from
0c04aaa to
9274fef
Compare
benesjan
left a comment
benesjan
left a comment
There was a problem hiding this comment.
Looks good. Feel free to merge once you address my comments
| }); | ||
|
|
||
| tokenSim.addAccount(walletGroup2[0].getAddress()); | ||
| tokenSim.setLookupProvider(walletGroup2[0].getAddress(), walletGroup2[0]); |
There was a problem hiding this comment.
Calling this addWallet would be much clearer. Lookup provider is too cryptic.
| amount, | ||
| nonce, | ||
| toAddressOption(accounts[0].address), | ||
| toAddressOption(wallets[0].getAddress()), |
There was a problem hiding this comment.
Is there some reason for why here we get the address from wallet and in the other 2 cases from accounts? The values should be the same, no?
|
|
||
| expect(wallets[0].getAddress()).toEqual(accounts[0].address); | ||
|
|
||
| const action = asset |
There was a problem hiding this comment.
| const action = asset | |
| // First we transfer funds to escrow and authorize it to donate on behalf of account 0 | |
| const action = asset |
Some comments would help here.
| ); | ||
| await wallets[0].createAuthWit({ caller: escrowContracts[0].address, action }); | ||
|
|
||
| await escrowContracts[0].withWallet(wallets[0]).methods.donate(amount, nonce).send().wait(); |
There was a problem hiding this comment.
| await escrowContracts[0].withWallet(wallets[0]).methods.donate(amount, nonce).send().wait(); | |
| // Now we check that donating from escrow works | |
| await escrowContracts[0].withWallet(wallets[0]).methods.donate(amount, nonce).send().wait(); |
|
|
||
| describe('failure cases', () => { | ||
| it('FAIL: direct transfer to vault fails', async () => { | ||
| const amount = 10n; |
There was a problem hiding this comment.
| const amount = 10n; | |
| // This test should fail because the escrow does not have any keys registered and hence obtaining them fails. | |
| const amount = 10n; |
Am I correct here that this is the cause of the issue?
| }); | ||
| }); | ||
|
|
||
| it('sending funds but using recipient as escrow, 🏦🤫💰', async () => { |
There was a problem hiding this comment.
This is a fun one. But the contracts could add checks to prevent this.
| it('sending funds but using recipient as escrow, 🏦🤫💰', async () => { | ||
| // For this test, I'm doing something quite strange, I am sending funds to Bob, but I am not allowing him to actually spend them | ||
| // I am still the "incoming_viewer_and_nullifier". This is a really annoying example as it will look like loss of funds to the users, and to get the funds | ||
| // back, or spend them, I need to give Bob some secrets 💀 |
There was a problem hiding this comment.
| // back, or spend them, I need to give Bob some secrets 💀 | |
| // back, or spend them, I need to give Bob my `nsk_app` and the `ivsk` (or give the note itself instead of `ivsk`).💀 |
Being more specific here is helpful.
| // In practice, the funds are still possible to rescue, but this makes our account a bit simpler. | ||
|
|
||
| { | ||
| const balance1 = await asset.methods.balance_of_private(accounts[1].address).simulate(); |
There was a problem hiding this comment.
Is accounts[1].address equal to walletGroup2[0].getAddress() here?
naming the addresses in this test would help.
| tokenSim.transferPrivate(accounts[0].address, AztecAddress.ZERO, amount); | ||
| // In practice, the funds are still possible to rescue, but this makes our account a bit simpler. | ||
|
|
||
| { |
There was a problem hiding this comment.
| { | |
| // Now we try to obtain the balance. It should fail with zero notes error because the recipient's PXE is expected to not have decrypted any notes. | |
| { |
| import { EscrowTokenContractTest, toAddressOption } from './escrowable_token_contract_test.js'; | ||
|
|
||
| // @todo For this test to be truly meaningful we need to run every actor with a separate PXE. | ||
| // when they are using the same we don't actually know if the default values are correct or |
There was a problem hiding this comment.
| // when they are using the same we don't actually know if the default values are correct or | |
| // When they are using the same we don't actually know if the default values are correct or |
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
2 times, most recently
from
797d971 to
449052a
Compare
LHerskind
force-pushed
the
lh/5864-escrow-token
branch
from
449052a to
0d8ca72
Compare
|
The consensus seems to be that we won't be following this direction as it can be handled at the app-layer instead and that it might just introduce "too" much complexity for usual transfers. |
4 participants
Initial escrow flow showcasing one way to address #5864
Replaces the blacklist token with an escrowable blacklist token. Additional tests cases of escrowing should be created to showcase the usage better in separate PR's.