Skip to content

bls: Refactor CBLSWrapper::SetBuf and CBLSWrapper::GetBuf#3867

Merged
xdustinface merged 6 commits into
dashpay:developfrom
xdustinface:pr-bls-drop-internal-methods
Dec 12, 2020
Merged

bls: Refactor CBLSWrapper::SetBuf and CBLSWrapper::GetBuf#3867
xdustinface merged 6 commits into
dashpay:developfrom
xdustinface:pr-bls-drop-internal-methods

Conversation

@xdustinface
Copy link
Copy Markdown

@xdustinface xdustinface commented Dec 10, 2020

Some refactoring of CBLSWrapper::GetBuf/CBLSWrapper::SetBuf to simplify the BLS classes a bit.

uint256 was implicit type of CBLSId but it doesn't provide FromBytes and the correct required Serialize to match the BLS primitives from dashpay/bls-signatures which are used by CBLSWrapper.

So 73d3271 + 2de99db are required to make 1df1d8b and 2c2e29d possible and b936f17 just drops not longer needed code.

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

I'm not sure I like the idea behind this refactoring... Like sure, it removes some LOC, but I'd say it makes using the Wrapper generally more difficult. (CBLSIdImplicit seems fine)

@xdustinface
Copy link
Copy Markdown
Author

I'd say it makes using the Wrapper generally more difficult.

How exactly? Can you explain what you mean?

Comment thread src/bls/bls.h Outdated
Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

This is definitely an improvement I think. Sure, it adds some additional requirements for ImplType (must implement Serialize(uint8_t*) and static ImplType FromBytes(const uint8_t*)) but it's completely fine IMO.

utACK

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

Okay, looking at it more, this does looks like a good improvement.

utACK 👍

@xdustinface xdustinface merged commit ec77d85 into dashpay:develop Dec 12, 2020
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 26, 2021
* bls: Add CBLSImplicit, a wrapper around uint256

This makes `CBLSImplicit` compatible (related to methods called by
CBLSWrapper) with the other classes from the
bls-signatures library.

* bls: Use CBLSImplicit instead of uint256 as base type of CBLSId

* bls: Use FromBytes directly instead of indirectly through InternalSetBuf

* bls: Use Serialize directly instead of indirectly through InternalGetBuf

* bls: Drop all occurrences of InternalSetBuf and InternalGetBuf

* bls: Use `CBLSIdImplicit` instead of `uint256` in some more places
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
7514361 Correct misleading "overridden options" label (Hennadii Stepanov)

Pull request description:

  Refs: dashpay#3867, bitcoin#8165.

Tree-SHA512: da3b13a0560654053aeff22a15031ae59a3136abc941f3959440c2d250add7de7ca837c96d721eed69b2cac21d340e1895a186f69383ab82a41fc1e0ee789e5c
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 14, 2022
* bls: Add CBLSImplicit, a wrapper around uint256

This makes `CBLSImplicit` compatible (related to methods called by
CBLSWrapper) with the other classes from the
bls-signatures library.

* bls: Use CBLSImplicit instead of uint256 as base type of CBLSId

* bls: Use FromBytes directly instead of indirectly through InternalSetBuf

* bls: Use Serialize directly instead of indirectly through InternalGetBuf

* bls: Drop all occurrences of InternalSetBuf and InternalGetBuf

* bls: Use `CBLSIdImplicit` instead of `uint256` in some more places
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.

3 participants