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.

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

Pass fee and crossover in execute to fix wrong transaction hash #85

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

Daksh14
Copy link
Contributor

@Daksh14 Daksh14 commented Oct 11, 2023

Use the same exact rng provided when generating output notes

Closes #84

Use the same exact rng provide when generating output notes
src/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

Some things I'd like to see changed or addressed. I second @Neotamandua 's comment.

Copy link
Member

Choose a reason for hiding this comment

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

This is a build artifact isn't it? As such it should not be added in this commit

Copy link
Member

Choose a reason for hiding this comment

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

There's still an artifact here. Do we really want to keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to keep it because one of the tests uses it

src/types.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
@Daksh14 Daksh14 force-pushed the fee_crossover_rng_fix branch 2 times, most recently from 70078b1 to e9ab926 Compare November 1, 2023 18:10
@Daksh14
Copy link
Contributor Author

Daksh14 commented Nov 1, 2023

We will need to regenerate the wasm with this PR and then release @ureeves

Ready for re-review

src/types.rs Show resolved Hide resolved
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

@Daksh14 and I had a talk and decided that the Type suffix is definitely a *nit, and since the WASM payload was already there, we can deal with it in a subsequent PR. Since those were the only two objections, I'm approving. LGTM!

@Daksh14
Copy link
Contributor Author

Daksh14 commented Nov 8, 2023

Follow up #94

@Daksh14 Daksh14 merged commit 74417ab into main Nov 8, 2023
4 checks passed
@Daksh14 Daksh14 deleted the fee_crossover_rng_fix branch November 8, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current wallet-core creates transactions which cannot be verified on rusk side
3 participants