Implement the TACo API module #263
Conversation
piotr-roslaniec
force-pushed
the
taco-api
branch
from
f340ff9
to
e08cfc2
Compare
piotr-roslaniec
force-pushed
the
set-default-variant
branch
from
752320c
to
36fd43e
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
from
e08cfc2
to
60af38b
Compare
Codecov Report
@@ Coverage Diff @@
## alpha #263 +/- ##
==========================================
- Coverage 81.37% 81.36% -0.01%
==========================================
Files 36 37 +1
Lines 1036 1068 +32
Branches 110 112 +2
==========================================
+ Hits 843 869 +26
- Misses 185 191 +6
Partials 8 8
|
piotr-roslaniec
force-pushed
the
taco-api
branch
from
60af38b
to
2d6d92e
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
from
2ccb4fa
to
b2cf2e4
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
from
352ae8c
to
7143d59
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
from
7143d59
to
db130b4
Compare
piotr-roslaniec
requested review from
jMyles,
KPrasch,
cygnusv,
theref,
derekpierre and
manumonti
piotr-roslaniec
force-pushed
the
taco-api
branch
from
861f715
to
3046d55
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
from
3046d55
to
5983ed0
Compare
src/taco.ts
Outdated
| // TODO: How do we get rid of these two fields? We need them for decrypting | ||
| // We ritualId in order to fetch the DKG participants and create DecryptionRequests for them | ||
| ritualId: number; | ||
| // We need to know the threshold in order to create DecryptionRequests | ||
| threshold: number; |
There was a problem hiding this comment.
Is there anything we can do to get rid of these parameters in the user-facing API?
There was a problem hiding this comment.
Do we want to expose class TacoMessageKit.requireSigner so that decrypting users can detect when they need to pass signer in?
There was a problem hiding this comment.
Given my comment above, I question whether this TacoMesageKit object is needed at all.
conditionExpr is in the ACP in the ThresholdMessageKit, and I don't believe ritualId and threshold are needed because they can be obtained from the Coordinator contract along with the ThresholdMessageKit.
My concern lies in returning objects that aren't subject to versioning, and the caller potentially serializing them. Ideally, encrypt would return a ThresholdMessageKit just like python.
Of course, if additional information is actually needed in the ThresholdMessageKit itself we can explore that.
piotr-roslaniec
force-pushed
the
taco-api
branch
2 times, most recently
from
8155c95
to
1921bc8
Compare
piotr-roslaniec
force-pushed
the
taco-api
branch
2 times, most recently
from
091a897
to
933416e
Compare
src/agents/coordinator.ts
Outdated
| word0: dkgPublicKeyBytes.slice(0, 16), | ||
| word1: dkgPublicKeyBytes.slice(16, 32), |
There was a problem hiding this comment.
A G1Point is 48 bytes and defined as follows.
Shouldn't word0 by (0,32) and word1 (32,48)?
There was a problem hiding this comment.
IMHO acceptance tests with contracts are unnecessary. We're only supporting read operations (at this point), and setting those up is not different than existing mocks (so, just having simple pass-through facades for mock values). Yes, this is a bug that went unnoticed. But it would have been caught easily by QA which we do on every release.
I think the better solution to this particular problem is to enhance the BLS12381.G1PointStructOutput generated type with a wrapper that performs validation. The word0 and word fields are represented as strings by typechain in generated typings, which also prevented the TS compiler from catching this issue.
There was a problem hiding this comment.
But it would have been caught easily by QA which we do on every release.
The goal of the "acceptance" tests is to catch these things before live QA. It is inefficient to catch these kind of bugs during live QA on a testnet. Doesn't mean we will catch everything in those tests, but it does reduce the need for subsequent fixes/releases after bugs are found during live testing.
My typescript philosophy knowledge is admittedly limited and I don't know what specifically the "acceptance" tests would entail and what it would require from the codebase to be set up, but having everything tested with mocks limits the breadth of testing and bugs that we can find in tests - who's testing the mocks is essentially the argument.
"Acceptance" tests is a broad term, and maybe there is more appropriate term here, but we should investigate how we can perform tests closer to what the actual testnet/production environment would be.
There was a problem hiding this comment.
I understand the advantages of acceptance testing, I just don't think they are necessary at this point. We should probably get them before the release, but not until we're done with the monorepo or adapting viem: #271
That being said, I'd be happy to accept a PR that implements acceptance testing if anyone wants to chip in.
There was a problem hiding this comment.
Definitely not something needed for this PR or this instant - but something we should nonetheless investigate. Filed #295 .
| aggregationMismatch: boolean; | ||
| accessController: string; | ||
| publicKey: BLS12381.G1PointStructOutput; | ||
| aggregatedTranscript: string; | ||
| } |
There was a problem hiding this comment.
Are participants not stored in this struct? I guess maybe not needed...?
There was a problem hiding this comment.
The contents of this struct are taken from types generated from contracts by typechain. I can trim them down to a size.
src/sdk/strategy/cbd-strategy.ts
Outdated
| const decrypter = ThresholdDecrypter.create( | ||
| porterUri, | ||
| dkgRitual.id, | ||
| dkgRitual.dkgParams.threshold |
There was a problem hiding this comment.
Is threshold still needed, even though it is now part of the ritual struct?
There was a problem hiding this comment.
The client needs to know how many shares to fetch from the nodes (/cbd_decrypt responses)
There was a problem hiding this comment.
Yes, but now if you have the ritual id, you can get the value of the threshold from the Coordinator agent when doing the retrieve, or am I misunderstanding.
There was a problem hiding this comment.
If you inspect the usage of DeployedCbdStrategy.create you can see that this is exactly what happens:
There was a problem hiding this comment.
Right 👍 . I mixed up my layers.
Side question: any reason we need the DkgRitualParameters object? This line could look nice as:
const decrypter = ThresholdDecrypter.create(
porterUri,
dkgRitual.id,
dkgRitual.threshold? Anyways, just something to think about.
| return await encryptLight(message, condition, dkgRitual.dkgPublicKey); | ||
| }; | ||
|
|
||
| export const encryptLight = async ( |
There was a problem hiding this comment.
Not sure how I feel about Light as the term 😅 . Can this just be an overloaded function with the same name?
There was a problem hiding this comment.
There is no support function overloading in JS/TS. I didn't have better ideas for an appropriate name, maybe we could use the blockchainy naming from the original TACo API issue.
There was a problem hiding this comment.
I don't have a good name for it, but I would prefer something like
export const encryptWithDKG = async (than the original one
piotr-roslaniec
force-pushed
the
taco-api
branch
from
933416e
to
f605e88
Compare
| return encrypter.encryptMessageCbd(toBytes(message), conditionExpr); | ||
| }; | ||
|
|
||
| export const decrypt = async ( |
piotr-roslaniec
force-pushed
the
taco-api
branch
from
f605e88
to
1dba09f
Compare
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
test/unit/taco.test.tsfor usage examples