Add report structs #29
Conversation
bamzedev
force-pushed
the
add-report-structs
branch
from
263c1cc to
f8d859e
Compare
|
I think it looks good so far. We are just missing adding the EG to the extrinsic right? |
yes |
|
@bamzedev if you could add the guarantees to the extrinsic, I think I would have everything I need 👍 |
|
|
||
| // Guarantee represents a single guarantee within the E_G extrinsic | ||
| type Guarantee struct { | ||
| CoreIndex uint32 // Index of the core this guarantee is for |
There was a problem hiding this comment.
From my understanding of the paper the core index is taken from the WorkReport.CoreIndex so defining it here is redundant
There was a problem hiding this comment.
The paper defines it that way. Not sure about the reason for this redundancy, maybe somewhere we will take this index without the need to deserialize the work report. Gavin knows for sure.
There was a problem hiding this comment.
in the paper the EG is defined as a tuple of 3 items, core index is not one of them
|
|
||
| // GuaranteesExtrinsic represents the E_G extrinsic | ||
| type GuaranteesExtrinsic struct { | ||
| Guarantees []Guarantee |
There was a problem hiding this comment.
Guarantees [NrOfCores]Guarantee
Where:
const NrOfCores = 341
?
There was a problem hiding this comment.
The same question as with Credential, do we need this intermediary struct GuaranteesExtrinsic or can we use []Guarantees directly?
There was a problem hiding this comment.
It is variable length, so we can't represent it and array with length 341.
There was a problem hiding this comment.
do we need this intermediary struct
I think so. I initially started the ticket with just a slice, but seems wrong and I have a PR fixing that. We will probably have functions specific for extrinsics, i.e. validate etc. So it makes sense.
|
|
||
| // Credential represents the credential a in the document | ||
| type Credential struct { | ||
| Signatures []CredentialSignature |
There was a problem hiding this comment.
Not trying to nitpick, but should we use []CredentialSignature directly in the Guarantee struct and rename CredentialSignature to Signature? we do we need to create this intermediary struct Credential?
There was a problem hiding this comment.
I think Signatures is correct, because you get them from a Credential struct, so: Credential.Signatures, instead of Credential.CredentialSignatures.
There was a problem hiding this comment.
My question is not as much regarding the naming, as if we need this Credential struct at all and if we should use the sequence of signatures in the parent struct directly
There was a problem hiding this comment.
Ah, sorry didn't read correctly. Yeah, doesn't make sense. I'll update it, thanks!
This PR adds the
EGextrinsic, akaReports.