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

feat: Use RPC to get MMR data for past blocks on note importing #337

Merged
merged 1 commit into from
May 15, 2024
Merged

feat: Use RPC to get MMR data for past blocks on note importing #337

merged 1 commit into from
May 15, 2024

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented May 13, 2024 •
edited
Loading

This PR implements the new GetBlockHeaderByNumber RPC changes. We can now ask for authentication data for past blocks, so whenever we import a note with verify, we can download the block header and authentication data as well.

@igamigo igamigo changed the base branch from igamigo-next-0.3 to igamigo-expected-notes May 13, 2024 02:07
@igamigo igamigo changed the base branch from igamigo-expected-notes to igamigo-next-0.3 May 13, 2024 02:08
@igamigo igamigo force-pushed the igamigo-get-mmr-data branch from f9f23f7 to 8ba4405 Compare May 13, 2024 03:03
@igamigo igamigo changed the base branch from igamigo-next-0.3 to igamigo-expected-notes May 13, 2024 03:03
@igamigo igamigo force-pushed the igamigo-get-mmr-data branch from 8ba4405 to 3f9b2a5 Compare May 13, 2024 03:57
@igamigo igamigo marked this pull request as ready for review May 13, 2024 14:01
mFragaBA
mFragaBA reviewed May 13, 2024
View reviewed changes
Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

In general looks good, I have a doubt regarding the merkle proof but once we figure that out we should be good to go. Other comments are very minor comments

src/client/notes.rs Outdated Show resolved Hide resolved
src/client/sync.rs Outdated Show resolved Hide resolved
src/client/sync.rs Outdated Show resolved Hide resolved
src/client/sync.rs Outdated Show resolved Hide resolved
tests/integration/common.rs Outdated Show resolved Hide resolved
Base automatically changed from igamigo-expected-notes to igamigo-next-0.3 May 13, 2024 19:16
mFragaBA
mFragaBA reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

Left a few more comments. Will test a bit with the CLI and come back with the results

src/client/sync.rs Outdated Show resolved Hide resolved
src/client/sync.rs Outdated Show resolved Hide resolved
src/client/sync.rs Outdated Show resolved Hide resolved
src/client/sync.rs Show resolved Hide resolved
mFragaBA
mFragaBA reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

Tested locally by creating a private p2id note, exporting it and imporing it in another client which had the target account. The information fetch was done properly and was able to consume the note!

mFragaBA
mFragaBA reviewed May 14, 2024
View reviewed changes
src/client/rpc/mod.rs Outdated Show resolved Hide resolved
mFragaBA
mFragaBA reviewed May 14, 2024
View reviewed changes
src/client/sync.rs Outdated Show resolved Hide resolved
@igamigo igamigo force-pushed the igamigo-get-mmr-data branch from 9cc98fc to 486d203 Compare May 14, 2024 23:38
mFragaBA
mFragaBA approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

LGTM!

@igamigo igamigo merged commit 7c1ad89 into igamigo-next-0.3 May 15, 2024
6 checks passed
@igamigo igamigo deleted the igamigo-get-mmr-data branch May 15, 2024 15:48
@mFragaBA
Copy link
Contributor

this PR closed #306

@mFragaBA mFragaBA linked an issue May 15, 2024 that may be closed by this pull request
Get block authentication information for past blocks #306
Closed
@mFragaBA mFragaBA mentioned this pull request May 15, 2024
Closed
@igamigo igamigo mentioned this pull request May 15, 2024
Merged
igamigo added a commit that referenced this pull request May 15, 2024
@igamigo @mFragaBA @tomyrd
feat: Integrate newer changes (#321)
f6f747d 
* feat: Integrate next changes

* fix: Back to next

* Fix conflicts

* Use node next

* fix: Tetss

* refactor: reuse function to create mocked client

* fix: update swap note script root

* fix: Generalize Authenticator

* Merge fixes and refactors

* Lints

* docs: update docs from changes to CLI (#336)

* feat: Utilize expected notes from `ExecutedTransaction` (#329)

* feat: Use expected notes from TransactionResult

* fix: Re-add output note check

* Remove unused code

* Lints

* Fix tests

* fix: Tetss

* Stash

* fix: Address reviews

* fix: Tests

* fix: Lints

* fix: Lints

* Try fix test

* fix: Merge conflicts

* Fix tests

* docs: Address reviews

* Lints

* fix: New script root

* Use 0.3

* fix: Correct genesis toml file

* fix: test fix con cargo make start-node

* fix: temp fix to rename config paths

* fix: Correct miden-node toml

* fix: Ship our own node config files

* Migrate to node 0.3

* feat: Use incoming MMR proof to add block authentications for notes created in past blocks (#337)

* test: Integration tests run with main node

* fix: Remove std::cmp::max

* refactor: git-like usage commands (#338)

* Move new account creation to command

* Flatten account command

* Refactor `default` option

* Separate new account commands

* Refactor `notes` command

* Refactor import comand to infer type

* Remove intermediate sucommand for export

* Flatten tags command

* Separate tx new commands

* Fix documentation

* Join p2id and p2idr to send command

* Remove trait

* Change comment suggestions

* Clarify new account help comments

* Ask for storage path if not specified

* Fixed problem with listing notes with no filter

* Added a message for default account updates

* Change suggestions

* Remove prompt for store path

* Added all NotesFilter

* Fix merge

* feat: enable swap notes (#324)

* feature: add swap tx request and store swap payback note

* feature: insert input note record from payback note details

* feature: show message to user on how to track payback notes

* fix: integration test compilation

* make swap tx send the offered asset

* fix: temporary fix for partial output notes

* test: add skeleton for integration test

* fix: lint issues

* test: add test for onchain swap flow

* test: add test for offchain swap flow

* test: fix note minting for swap tests

* test: fix compilation errors

* fix: use all committed note ids to check transactions to commit

* fix: fix tests checks

* address review comments

* docs: update CHANGELOG

* docs: update CLI reference

* address remaining comment

* fix: clippy issues

* fix: remove wrong check

* refactor: store partial notes in relevant notes of transaction result

* rephrase doc comment

Co-authored-by: igamigo <[email protected]>

* address review comments

* fix: fix offchain swap test

---------

Co-authored-by: igamigo <[email protected]>

* docs: fix doc comments for swap clap command

* feat: update cli-reference with new command structure (#342)

* Update cli-reference with new command structure

* Fix merge

* Fix merge

* Update cli-reference.md

---------

Co-authored-by: igamigo <[email protected]>

* Update README.md

* CLI config.md

---------

Co-authored-by: Martin Fraga <[email protected]>
Co-authored-by: Martin Fraga <[email protected]>
Co-authored-by: Tomas Rodriguez Dala <[email protected]>
bobbinth pushed a commit that referenced this pull request May 15, 2024
@igamigo @mFragaBA
@tomyrd @bobbinth
feat: Integrate newer changes (#321)
351b0d4 
* feat: Integrate next changes

* fix: Back to next

* Fix conflicts

* Use node next

* fix: Tetss

* refactor: reuse function to create mocked client

* fix: update swap note script root

* fix: Generalize Authenticator

* Merge fixes and refactors

* Lints

* docs: update docs from changes to CLI (#336)

* feat: Utilize expected notes from `ExecutedTransaction` (#329)

* feat: Use expected notes from TransactionResult

* fix: Re-add output note check

* Remove unused code

* Lints

* Fix tests

* fix: Tetss

* Stash

* fix: Address reviews

* fix: Tests

* fix: Lints

* fix: Lints

* Try fix test

* fix: Merge conflicts

* Fix tests

* docs: Address reviews

* Lints

* fix: New script root

* Use 0.3

* fix: Correct genesis toml file

* fix: test fix con cargo make start-node

* fix: temp fix to rename config paths

* fix: Correct miden-node toml

* fix: Ship our own node config files

* Migrate to node 0.3

* feat: Use incoming MMR proof to add block authentications for notes created in past blocks (#337)

* test: Integration tests run with main node

* fix: Remove std::cmp::max

* refactor: git-like usage commands (#338)

* Move new account creation to command

* Flatten account command

* Refactor `default` option

* Separate new account commands

* Refactor `notes` command

* Refactor import comand to infer type

* Remove intermediate sucommand for export

* Flatten tags command

* Separate tx new commands

* Fix documentation

* Join p2id and p2idr to send command

* Remove trait

* Change comment suggestions

* Clarify new account help comments

* Ask for storage path if not specified

* Fixed problem with listing notes with no filter

* Added a message for default account updates

* Change suggestions

* Remove prompt for store path

* Added all NotesFilter

* Fix merge

* feat: enable swap notes (#324)

* feature: add swap tx request and store swap payback note

* feature: insert input note record from payback note details

* feature: show message to user on how to track payback notes

* fix: integration test compilation

* make swap tx send the offered asset

* fix: temporary fix for partial output notes

* test: add skeleton for integration test

* fix: lint issues

* test: add test for onchain swap flow

* test: add test for offchain swap flow

* test: fix note minting for swap tests

* test: fix compilation errors

* fix: use all committed note ids to check transactions to commit

* fix: fix tests checks

* address review comments

* docs: update CHANGELOG

* docs: update CLI reference

* address remaining comment

* fix: clippy issues

* fix: remove wrong check

* refactor: store partial notes in relevant notes of transaction result

* rephrase doc comment

Co-authored-by: igamigo <[email protected]>

* address review comments

* fix: fix offchain swap test

---------

Co-authored-by: igamigo <[email protected]>

* docs: fix doc comments for swap clap command

* feat: update cli-reference with new command structure (#342)

* Update cli-reference with new command structure

* Fix merge

* Fix merge

* Update cli-reference.md

---------

Co-authored-by: igamigo <[email protected]>

* Update README.md

* CLI config.md

---------

Co-authored-by: Martin Fraga <[email protected]>
Co-authored-by: Martin Fraga <[email protected]>
Co-authored-by: Tomas Rodriguez Dala <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mFragaBA mFragaBA mFragaBA approved these changes

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

Successfully merging this pull request may close these issues.

Get block authentication information for past blocks
2 participants
@igamigo @mFragaBA