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

Improve amount decomposer fail condition for Trezor integration tests #11671

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Improve amount decomposer fail condition for Trezor integration tests #11671

merged 1 commit into from
Oct 17, 2023

Conversation

M1nd3r
Copy link
Contributor

@M1nd3r M1nd3r commented Oct 11, 2023

No description provided.

turbolay
turbolay reviewed Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

It's ok but I think it's better to create a new block, because the message thrown is not adapted to this case.

Also, this file has a specific process to be updated: PR must be merged first on Sake's side, so please apply your patch here: https://github.com/nopara73/Sake/blob/0188e5b3815e40e6304ec192dbfc91a6447cddf4/Sake/Mixer.cs#L104

Sake is a simulator to test the amount decomposition.

@M1nd3r
Copy link
Contributor Author

M1nd3r commented Oct 13, 2023 •
edited

Thanks for feedback, I have pushed adjusted changes to the nopara73/Sake project.
nopara73/Sake#44

@M1nd3r M1nd3r closed this Oct 13, 2023
@lontivero lontivero reopened this Oct 17, 2023
@lontivero lontivero merged commit 94299fc into WalletWasabi:master Oct 17, 2023
8 checks passed
@lontivero lontivero added the affiliate Could affect affiliates compatibility label Oct 17, 2023
@M1nd3r
Copy link
Contributor Author

M1nd3r commented Oct 17, 2023

@lontivero Hi. I would like to know why did you merged this pull request? In Sake, the problem is now solved differently (as a separate if block with different exception message). I do not know, if this inconsistency could cause some problems down the road. I just want to let you know, that such inconsistency exists.

@lontivero
Copy link
Collaborator

I saw it merged and I wrongly assumed it was the same solution.

lontivero added a commit that referenced this pull request Oct 17, 2023
@lontivero
Revert "Change amount decomposer conition for Trezor tests (#11671)"
72024a9 
This reverts commit 94299fc.
@lontivero lontivero mentioned this pull request Oct 17, 2023
Merged
lontivero added a commit that referenced this pull request Oct 17, 2023
@lontivero
Revert "Change amount decomposer conition for Trezor tests (#11671)" (#…
73da3b7 
…11715)

This reverts commit 94299fc.
@lontivero
Copy link
Collaborator

@M1nd3r could you make a new PR here, please?

@M1nd3r
Copy link
Contributor Author

M1nd3r commented Oct 17, 2023

Sure, I'll do it first thing in the morning.

@M1nd3r
Copy link
Contributor Author

M1nd3r commented Oct 18, 2023

@lontivero There is the new PR: #11719

@M1nd3r M1nd3r deleted the M1nd3r/fixConditionForTrezorIntegrationTests branch October 18, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolay turbolay turbolay left review comments

Assignees
No one assigned
Labels
affiliate Could affect affiliates compatibility size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@M1nd3r @lontivero @turbolay