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

wallet-core: Add functionality to create transaction #2135

Merged
merged 15 commits into from
Aug 26, 2024
Merged

wallet-core: Add functionality to create transaction #2135

merged 15 commits into from
Aug 26, 2024

Conversation

moCello
Copy link
Member

@moCello moCello commented Aug 14, 2024 •
edited
Loading

To be able to create transactions the rusk-prover needed to be updated as well.
The rusk-prover::Prover trait is replaced by the execution_core::transfer::phoenix::Prove trait.
With this change, the rusk-prover library now only holds the LocalProver implementation of the Prove trait and the feature structure was drastically simplified.

Additionally I took the liberty to also adjust the names of the cached TxCircuit circuit-profile, prover and verifier in order for them to better match their actual circuit`s name.

The rusk http prover request also changed from "prove_execute" to "prove_tx_circuit".

Resolves #2128

@moCello moCello force-pushed the mocello/2128_wallet_tx branch from ac185e0 to 242cf56 Compare August 20, 2024 17:25
@moCello moCello marked this pull request as ready for review August 22, 2024 15:39
@moCello moCello force-pushed the mocello/2128_wallet_tx branch 2 times, most recently from 4f490f1 to e45e0a7 Compare August 23, 2024 10:52
Daksh14
Daksh14 requested changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@Daksh14 Daksh14 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One suggestion and tests for transactions would be nice, an example of sending a ping/pong tx to the node in the examples folder also will be helpful otherwise LGTM, once the change we discussed is done I'll approve

wallet-core/src/transaction.rs Show resolved Hide resolved
wallet-core/src/transaction.rs Outdated Show resolved Hide resolved
@moCello moCello force-pushed the mocello/2128_wallet_tx branch from e45e0a7 to daf8a08 Compare August 23, 2024 11:49
@moCello moCello force-pushed the mocello/2128_wallet_tx branch from daf8a08 to cd65045 Compare August 23, 2024 11:57
HDauven
HDauven previously approved these changes Aug 23, 2024
View reviewed changes
@moCello moCello force-pushed the mocello/2128_wallet_tx branch from cd65045 to fde6e09 Compare August 23, 2024 12:29
HDauven
HDauven previously approved these changes Aug 23, 2024
View reviewed changes
herr-seppia
herr-seppia requested changes Aug 23, 2024
View reviewed changes
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Apart from some nit, I would like to keep the prover trait

execution-core/src/transfer/phoenix.rs Outdated Show resolved Hide resolved
contracts/transfer/build.rs Outdated Show resolved Hide resolved
rusk-prover/src/errors.rs Outdated Show resolved Hide resolved
rusk-prover/src/lib.rs Show resolved Hide resolved
rusk-prover/src/lib.rs Show resolved Hide resolved
ZER0
ZER0 previously approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@ZER0 ZER0 left a comment

Choose a reason for hiding this comment

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

A couple of things to change, but LGTM

execution-core/src/transfer/phoenix.rs Outdated Show resolved Hide resolved
execution-core/src/transfer/phoenix.rs Outdated Show resolved Hide resolved
herr-seppia
herr-seppia requested changes Aug 24, 2024
View reviewed changes
rusk/src/lib/http/prover.rs Outdated Show resolved Hide resolved
@moCello moCello marked this pull request as draft August 26, 2024 07:41
@moCello moCello dismissed stale reviews from ZER0 and HDauven via e0f54bd August 26, 2024 13:07
@moCello moCello marked this pull request as ready for review August 26, 2024 13:11
@moCello moCello linked an issue Aug 26, 2024 that may be closed by this pull request
execution-core: Check for duplicate notes at transaction-creation #2204
Closed
@moCello moCello mentioned this pull request Aug 26, 2024
Closed
@moCello moCello force-pushed the mocello/2128_wallet_tx branch from e0f54bd to 673a8eb Compare August 26, 2024 13:33
herr-seppia
herr-seppia previously approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

ZER0
ZER0 previously approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@ZER0 ZER0 left a comment

Choose a reason for hiding this comment

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

LGTM there is only a left over on the display of the error, I believe (check my comment)

execution-core/src/error.rs Outdated Show resolved Hide resolved
@moCello moCello dismissed stale reviews from ZER0 and herr-seppia via 24da333 August 26, 2024 14:08
@moCello moCello force-pushed the mocello/2128_wallet_tx branch from 673a8eb to 24da333 Compare August 26, 2024 14:08
@HDauven HDauven requested review from Daksh14 and removed request for Daksh14 August 26, 2024 14:42
@Daksh14
Copy link
Contributor

Daksh14 commented Aug 26, 2024

LGTM

@HDauven HDauven merged commit 08fb392 into master Aug 26, 2024
15 checks passed
@HDauven HDauven deleted the mocello/2128_wallet_tx branch August 26, 2024 14:54
@moCello moCello mentioned this pull request Aug 27, 2024
Closed
moCello added a commit that referenced this pull request Oct 2, 2024
@moCello
rusk-prover: Add debug feature to print the test circuit-bytes
b1a5c37 
This behaviour was lost with the merge of PR #2135
@moCello moCello mentioned this pull request Oct 2, 2024
Merged
moCello added a commit that referenced this pull request Oct 2, 2024
@moCello
rusk-prover: Add debug feature to print the test circuit-bytes
3bd27b8 
This behaviour was lost with the merge of PR #2135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Daksh14 Daksh14 Daksh14 approved these changes

@ZER0 ZER0 ZER0 approved these changes

@herr-seppia herr-seppia herr-seppia left review comments

@miloszm miloszm Awaiting requested review from miloszm

@ureeves ureeves Awaiting requested review from ureeves

@HDauven HDauven Awaiting requested review from HDauven

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

Successfully merging this pull request may close these issues.

execution-core: Check for duplicate notes at transaction-creation wallet-core: create transactions
5 participants
@moCello @Daksh14 @ZER0 @HDauven @herr-seppia