Remove client dependency from faucet #368

tomyrd · 2024-05-27T22:00:38Z

closes #343

This PR removes almost all of the functionality implemented by the miden-client and replaces it with it's own implementation that doesn't depend on the crate. The new FaucetClient is stored in the state and wrapped with a mutex to allow concurrency (a feature that was removed in the previous version).

Some caveats:

We still have a small dependency. The InputNoteRecord type is needed in order to be serialized and imported by the user. We could change the miden-client to accept the Note type as it's import or move the InputNoteRecord to miden-base.
There's a small issue where if multiple transactions are sent at the same time, the node accepts the first one and rejects all others. This happens because of this verification.

igamigo

Looking good! Left some comments

bin/faucet/src/client.rs

bin/faucet/Cargo.toml

bobbinth · 2024-05-29T05:19:12Z

We still have a small dependency. The InputNoteRecord type is needed in order to be serialized and imported by the user. We could change the miden-client to accept the Note type as it's import or move the InputNoteRecord to miden-base.

I didn't see where in the code we are currently using InputNoteRecord, but if we need it, my preference would be to create a struct in miden-objects (e.g., maybe something like NoteFile which is used specifically for serializing different note variants.

tomyrd · 2024-05-29T15:35:14Z

I didn't see where in the code we are currently using InputNoteRecord

Here. It's needed because the client can only import serialized InputNoteRecord files.

maybe something like NoteFile which is used specifically for serializing different note variants.

Created an issue for this in miden-base.

mFragaBA

Not a final review, In general looks good though. I'll continue with testing these changes locally.

One thing I noticed while reviewing this is that we never store the faucet state. This means that if by any reason the server crashes I lost the faucet forever. It might be worth considering storing it somewhere, but it's a separate issue than this one.

mFragaBA · 2024-05-29T19:07:33Z

bin/faucet/src/client.rs

+
+#[derive(Clone)]
+pub struct FaucetDataStore {
+    faucet_account: Rc<RefCell<Account>>,


If we changed Rc<RefCell<Account>> for Arc<<Mutex<Account>> (maybe even Arc<RwLock<Account>>) would we still need the unsafe impl Send and unsafe impl Sync from above?

Yes because the TransactionExecutor still wouldn't be Send+Sync

mFragaBA · 2024-05-29T19:10:40Z

bin/faucet/src/client.rs

+
+struct FaucetAuthenticator {
+    secret_key: AuthSecretKey,
+    rng: RefCell<ChaCha20Rng>,


as for what was said above, what happens if we wrap this with a Mutex/ RwLock instead of a RefCell?

It was suggested to remove this struct in favor of the BasicAuthenticator which currently also uses a RefCell. In the future this may change and we could remove the unsafe impl Send+Sync.

bin/faucet/src/handlers.rs

mFragaBA

Tested locally with the dockerized node, worked like a charm. In fact, it feels faster than before (it's still restricted by block time since we can only do one tx per block.

Again unrelated to the PR, but could it be possible to do a mint for multiple accounts at a time by "aggregating" many mint requests into a single Tx that generates all mint notes? It would probably change the flow a bit since we'd need to switch to a polling scheme or similar, but it would allow the faucet to mint for several accounts per block.

bobbinth

Looks good! Thank you! I left a couple of small comments inline.

Also, I think as a part of this PR we should get rid of the miden-client dependency entirely. So, before merging this, we should probably address 0xPolygonMiden/miden-base#715 and propagate the changes here and to miden-client.

bin/faucet/src/client.rs

tomyrd · 2024-05-31T16:17:28Z

So, before merging this, we should probably address 0xPolygonMiden/miden-base#715 and propagate the changes here and to miden-client.

Creted a PR adding the new NoteFile object. Drafting this PR until that one is merged so as to not merge it by accident.

We will also need to update the client to use the new serialized file but it's not a blocking change for the faucet.

bobbinth

Thank you! Not a very in-depth review - but overall everything looks good (only a couple of inline comments from my end).

bin/faucet/Cargo.toml

bobbinth · 2024-06-04T15:24:54Z

@tomyrd - thank you!

@igamigo, @mFragaBA - could one of you take another look and then we can merge?

igamigo

LGTM! Left a couple of comments but nothing blocking.

bin/faucet/src/client.rs

This reverts commit b103d38.

bobbinth · 2024-06-06T18:17:08Z

I see that this was merged into mFragaBA-next-0.4. Should we merge it into next?

bin/faucet/Cargo.toml

mFragaBA · 2024-06-06T18:35:38Z

I see that this was merged into mFragaBA-next-0.4. Should we merge it into next?

it will change the miden-base dependencies to point to the next branch since it was merged into mFragaBA-next-0.4, if we're ok with that then sure (we'll need to change it before release)

bobbinth · 2024-06-06T18:55:32Z

it will change the miden-base dependencies to point to the next branch since it was merged into mFragaBA-next-0.4, if we're ok with that then sure (we'll need to change it before release)

Yes - I think it is fine to have next branch here point to the next branch in miden-base.

bobbinth

Looks good! Thank you! I left one comment inline.

crates/store/src/state.rs

mFragaBA

LGTM! (tested locally with a dockerized node + faucet and an account from a local client. Created multiple public notes and was able to consume them). We'd still need 0xPolygonMiden/miden-client#375 for private ones to import private notes into the client

tomyrd marked this pull request as ready for review May 28, 2024 18:45

tomyrd requested review from bobbinth, igamigo and mFragaBA May 28, 2024 18:48

igamigo reviewed May 28, 2024

View reviewed changes

bobbinth reviewed May 29, 2024

View reviewed changes

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

bobbinth reviewed May 29, 2024

View reviewed changes

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

tomyrd mentioned this pull request May 29, 2024

Add type for serialized notes 0xPolygonMiden/miden-base#715

Closed

tomyrd requested review from igamigo and bobbinth May 29, 2024 15:36

mFragaBA reviewed May 29, 2024

View reviewed changes

bobbinth reviewed May 30, 2024

View reviewed changes

bin/faucet/src/client.rs Outdated Show resolved Hide resolved

bin/faucet/src/client.rs Show resolved Hide resolved

tomyrd force-pushed the tomyrd-remove-client-from-faucet branch from 2dc9fc9 to cb9e735 Compare May 31, 2024 15:57

tomyrd marked this pull request as draft May 31, 2024 16:17

tomyrd requested review from mFragaBA and bobbinth May 31, 2024 16:17

tomyrd force-pushed the tomyrd-remove-client-from-faucet branch from 0a2aff6 to b0e9116 Compare June 3, 2024 19:43

tomyrd changed the base branch from next to mFragaBA-next-0.4 June 3, 2024 19:45

tomyrd force-pushed the tomyrd-remove-client-from-faucet branch from b0e9116 to 0a2aff6 Compare June 3, 2024 19:48

mFragaBA and others added 2 commits June 3, 2024 17:02

fix compilation error from pointing to base's next

bd54a42

Fix database test

255ea57

tomyrd force-pushed the mFragaBA-next-0.4 branch from ac002f8 to 255ea57 Compare June 3, 2024 20:04

tomyrd added 4 commits June 3, 2024 17:05

Add new faucet client struct

a22e635

Add FaucetAuthenticator

d5a7055

Add executor to FaucetClient

a7b8693

Remove client dependency

a72a012

tomyrd added 4 commits June 3, 2024 17:07

Remove FaucetAuthenticator in favor of BasicAuthenticator

91c2c86

Add section separators

502a726

Add doc comments

2b6be13

Add comments to faucet state

a176307

tomyrd force-pushed the tomyrd-remove-client-from-faucet branch from 0a2aff6 to a176307 Compare June 3, 2024 20:08

tomyrd added 2 commits June 3, 2024 18:04

Use next miden-base from faucet

18043fd

Remove miden-client dependency

39abe87

tomyrd marked this pull request as ready for review June 3, 2024 21:59

bobbinth approved these changes Jun 4, 2024

View reviewed changes

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

tomyrd added 2 commits June 4, 2024 11:57

Fix rng in faucet client

b103d38

Fix Cargo.toml

03f4a9e

igamigo approved these changes Jun 4, 2024

View reviewed changes

bin/faucet/src/client.rs Outdated Show resolved Hide resolved

bin/faucet/src/client.rs Outdated Show resolved Hide resolved

Fix doc comment for get_random_coin

7851871

tomyrd mentioned this pull request Jun 5, 2024

fix: change rngtype to &mut R in note creation functions 0xPolygonMiden/miden-base#733

Merged

Revert "Fix rng in faucet client"

3865e01

This reverts commit b103d38.

mFragaBA reviewed Jun 6, 2024

View reviewed changes

bin/faucet/Cargo.toml Outdated Show resolved Hide resolved

tomyrd changed the base branch from mFragaBA-next-0.4 to next June 6, 2024 19:16

tomyrd added 2 commits June 6, 2024 16:41

Make tonic a workspace dependency

387be69

Merge branch 'next' into tomyrd-remove-client-from-faucet

52fe8fc

bobbinth approved these changes Jun 6, 2024

View reviewed changes

crates/store/src/state.rs Show resolved Hide resolved

mFragaBA approved these changes Jun 6, 2024

View reviewed changes

bobbinth merged commit 0bd0ee4 into next Jun 7, 2024
6 checks passed

bobbinth deleted the tomyrd-remove-client-from-faucet branch June 7, 2024 06:38

This was referenced Jun 7, 2024

Remove client dependency from the faucet #343

Closed

Return the list of committed transactions in state sync endpoint #377

Merged

bobbinth pushed a commit that referenced this pull request Jun 10, 2024

feat: remove client dependency from faucet (#368)

57cc003

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove client dependency from faucet #368

Remove client dependency from faucet #368

tomyrd commented May 27, 2024 •

edited

Loading

igamigo left a comment

bobbinth commented May 29, 2024

tomyrd commented May 29, 2024

mFragaBA left a comment

mFragaBA May 29, 2024

tomyrd May 30, 2024

mFragaBA May 29, 2024

tomyrd May 30, 2024

mFragaBA left a comment

bobbinth left a comment

tomyrd commented May 31, 2024 •

edited

Loading

bobbinth left a comment

bobbinth commented Jun 4, 2024

igamigo left a comment

bobbinth commented Jun 6, 2024

mFragaBA commented Jun 6, 2024

bobbinth commented Jun 6, 2024

bobbinth left a comment

mFragaBA left a comment •

edited

Loading

Remove client dependency from faucet #368

Remove client dependency from faucet #368

Conversation

tomyrd commented May 27, 2024 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

bobbinth commented May 29, 2024

tomyrd commented May 29, 2024

mFragaBA left a comment

Choose a reason for hiding this comment

mFragaBA May 29, 2024

Choose a reason for hiding this comment

tomyrd May 30, 2024

Choose a reason for hiding this comment

mFragaBA May 29, 2024

Choose a reason for hiding this comment

tomyrd May 30, 2024

Choose a reason for hiding this comment

mFragaBA left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

tomyrd commented May 31, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth commented Jun 4, 2024

igamigo left a comment

Choose a reason for hiding this comment

bobbinth commented Jun 6, 2024

mFragaBA commented Jun 6, 2024

bobbinth commented Jun 6, 2024

bobbinth left a comment

Choose a reason for hiding this comment

mFragaBA left a comment • edited Loading

Choose a reason for hiding this comment

tomyrd commented May 27, 2024 •

edited

Loading

tomyrd commented May 31, 2024 •

edited

Loading

mFragaBA left a comment •

edited

Loading