-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Pedersen commitment improvements #182
Conversation
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
========================================
+ Coverage 12.5% 16.1% +3.6%
========================================
Files 31 31
Lines 3188 3379 +191
========================================
+ Hits 400 544 +144
- Misses 2788 2835 +47
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow... Different semantic ID for the core RGB lib. Is this a pushback or a fast forward? How bad is this breaking? Is this as bad as 0.10 was?
If we do any breaking changes, we need to do them now.
Neither. It is a bugfix, meaning the existing way Pedersen commitments are done was non-standard and may contain inflation issues. I was seeking for Blockstream review of the way we use their API for Pedersen commitments and only now do we have it, thus we need this fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's definitely get this in ASAP.
It is not the only change which is needed - there are a bunch of other required PRs some of which are breaking. Also, we need to get RGB and std libraries updated and ensure that all PSBT methods create properly balanced Pedersen commitments and tapret commitments. Thus please no releases before that is accomplished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments requiring minor changes, but overall LGTM
Co-authored-by: Zoe Faltibà <7492268+zoedberg@users.noreply.github.com>
5d4e9bd
to
f2471fe
Compare
@zoedberg thank you for the good point with tests. I have committed your doc fixes, fixed different lints and resolved conflicts with master branch as well. |
According to the suggestions by Adam Back and Blockstream team.
API changes are breaking, thus it can go only into v0.11