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.

Sign up for GitHub

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

Jump to bottom

Add support for on-chain notes to block-producer #310

Merged
merged 18 commits into from
Apr 11, 2024
Merged

Add support for on-chain notes to block-producer #310

merged 18 commits into from
Apr 11, 2024

Conversation

phklive
Copy link
Contributor

@phklive phklive commented Apr 10, 2024

In this PR I propose to update the block-proposer to enable addition of on-chain notes to the store through apply_block api

Closes: #141

@phklive phklive requested a review from polydez April 10, 2024 09:58
@phklive phklive mentioned this pull request Apr 10, 2024
Closed
polydez
polydez requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@polydez polydez left a comment

Choose a reason for hiding this comment

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

Thanks you! I left several inline comments. The main idea here that we don't need to produce NoteCreated in the batch builder when we don't ready to fill its fields, but later in the block producer. We also can get rid of several fields.

block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/block.rs Outdated Show resolved Hide resolved
block-producer/src/block_builder/mod.rs Outdated Show resolved Hide resolved
@phklive phklive marked this pull request as ready for review April 10, 2024 13:19
polydez
polydez requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@polydez polydez left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! I've left a couple comments here. Please also remove unused conversion from module proto/src/domain/notes.rs.

block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/block_builder/mod.rs Outdated Show resolved Hide resolved
polydez
polydez reviewed Apr 10, 2024
View reviewed changes
proto/src/domain/notes.rs Outdated Show resolved Hide resolved
igamigo
igamigo approved these changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

LGTM

@phklive phklive requested a review from polydez April 10, 2024 21:02
bobbinth
bobbinth requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of comments inline which I think should simplify things a little.

Also, as a part of this PR, let's also address this TODO.

block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
block-producer/src/block.rs Outdated
@@ -19,7 +19,7 @@ use crate::store::BlockInputsError;
pub struct Block {
pub header: BlockHeader,
pub updated_accounts: Vec<AccountUpdateDetails>,
pub created_notes: Vec<(usize, usize, NoteEnvelope)>,
pub created_notes: Vec<NoteCreated>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the previous comment: this could have probably have been something like Vec<(usize, usize, NoteEnvelope)>. In general, I think we should try to avoid using protobuf types inside domain types.

A thought for the future: we should probably move the Block struct into miden-base at some point.

@bobbinth
Copy link
Contributor

I've merged @igamigo PR into this. There we introduced this temporary change. I think we should address it properly as a part of this PR.

@bobbinth
Copy link
Contributor

Also, as a part of this PR, let's also address this TODO.

Seems like @igamigo has already addressed this TODO in on of the commits in this PR.

@polydez polydez requested a review from bobbinth April 11, 2024 09:32
bobbinth
bobbinth reviewed Apr 11, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

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

store/src/db/mod.rs Outdated Show resolved Hide resolved
block-producer/src/block.rs Outdated Show resolved Hide resolved
@polydez polydez requested a review from bobbinth April 11, 2024 17:18
@mFragaBA
Copy link
Contributor

@polydez can we pull next into this? There's a fix we need to run the client's integration tests (we're currently pointing to this branch to develop and without the fix the integration tests run indefinitely on the CI)

@polydez
Copy link
Contributor

polydez commented Apr 11, 2024

@polydez can we pull next into this? There's a fix we need to run the client's integration tests (we're currently pointing to this branch to develop and without the fix the integration tests run indefinitely on the CI)

Sure!

bobbinth
bobbinth reviewed Apr 11, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some more comments inline. Some of these will require some changes in miden-base. I'll work on these and then we can finish this PR.

proto/proto/note.proto Show resolved Hide resolved
proto/proto/note.proto Show resolved Hide resolved
proto/proto/note.proto Outdated Show resolved Hide resolved
store/src/server/api.rs Outdated Show resolved Hide resolved
store/src/db/sql.rs Outdated Show resolved Hide resolved
store/src/db/sql.rs Outdated Show resolved Hide resolved
bobbinth
bobbinth approved these changes Apr 11, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of test-related comments inline - but they are non-blocking.

block-producer/src/test_utils/block.rs Outdated Show resolved Hide resolved
block-producer/src/test_utils/block.rs Outdated Show resolved Hide resolved
@bobbinth bobbinth merged commit 6aa2c90 into next Apr 11, 2024
5 checks passed
@bobbinth bobbinth deleted the phklive-block-producer-onchain-notes branch April 11, 2024 22:05
bobbinth pushed a commit that referenced this pull request Apr 12, 2024
@phklive @igamigo
@polydez @bobbinth
feat: add support for on-chain notes to block-producer (#310)
0c831fb 
Co-authored-by: igamigo <[email protected]>
Co-authored-by: polydez <[email protected]>
@Dominik1999 Dominik1999 mentioned this pull request Apr 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igamigo igamigo igamigo approved these changes

@bobbinth bobbinth bobbinth approved these changes

@polydez polydez Awaiting requested review from polydez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phklive @bobbinth @mFragaBA @polydez @igamigo