Small cleanup and fixes #76
Conversation
libzecale/zecale_constants.hpp
Outdated
| @@ -2,16 +2,21 @@ | |||
| // | |||
| // SPDX-License-Identifier: LGPL-3.0+ | |||
|
|
|||
| #ifndef __ZECALE_ZECALE_CONSTANTS__ | |||
| #define __ZECALE_ZECALE_CONSTANTS__ | |||
| #ifndef __ZECALE_ZECALE_CONSTANTS_HPP__ | |||
There was a problem hiding this comment.
libzecale should probably be completely agnostic to these constants, so maybe they would be better in the aggregator_server directory?
There was a problem hiding this comment.
I followed a similar approach to what we did in Zeth: https://github.com/clearmatics/zeth/blob/develop/libzeth/zeth_constants.hpp here.
I think moving these in the aggregator_server directory is a good idea, but we need to find the boundary between what constitutes a "protocol param" and what's part of the backend/server config (that also applies to the Zeth-side then (but only these 3 vars would be potentially moved: https://github.com/clearmatics/zeth/blob/develop/libzeth/zeth_constants.hpp#L13-L16 I guess)).
Here (in Zecale), the libzecale::ZECALE_NUM_INPUTS_PER_NESTED_PROOF definitely is a protocol param to me (as part of the protocol spec assumes underlying app hash their instance). The batch size however may be seen as a server config (along the lines of #75)
There was a problem hiding this comment.
TBH - I would almost be tempted to move these to the CMakeLists file (along with the curve selection) to define the protocol params in the build config...
There was a problem hiding this comment.
Yes, makes sense to move those 3 constants in zeth as well.
No problem with the batch size etc being defined by CMake if thats easy enough. It would be ideal if they were out of scope during the build of libzecale (similaly for libzeth), i.e. it could be good to put them somewhere other than build/zecale.h. But that can be fixed up later if it's awkward.
There was a problem hiding this comment.
As noted offline by @AntoineRondelet, libzecale::ZECALE_NUM_INPUTS_PER_NESTED_PROOF is not necessarily a simple independent constant. I'll create an issue to resolve this in the future.
There was a problem hiding this comment.
I'll create an issue to resolve this in the future.
Thanks @dtebbs
As per our chat, I dropped the commit that used the libzecale:: constants in the aggregator_server to avoid "non-trivial" changes before v0.4. This reverts to the use of the static consts in the aggregator_server.cpp. I also removed the libzecale/zecale_constants.hpp file which was unused as of now. Let's see if we need to re-introduce it later on.
There was a problem hiding this comment.
Great. For now, I've created #79 to resolve this later.
AntoineRondelet
force-pushed
the
cleanup-and-fixes
branch
from
6452530
to
0ed4630
Compare
No description provided.