Skip to content
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

contract data availability proofs & updated types & bug fixes #24

Merged
merged 12 commits into from
Apr 3, 2024

Conversation

Geo25rey
Copy link
Member

@Geo25rey Geo25rey commented Apr 2, 2024

when merging please rebase instead of merging. the merge commit messes up the git history on my fork

see commit messages for detailed changes

Copy link
Member

@vaultec81 vaultec81 left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few comments

src/services/new/contractEngineV2.ts Outdated Show resolved Hide resolved
type: 'proof'
data: string
signature: { sig: string; bv: string }
epochDefinitionHiveBlock: number // just some metadata for the client to aid verification
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to epochHeight or just epoch is the convention I'm going for.

Comment on lines 281 to 286
{
type: 'data-availablity',
cid: respInfo.cid,
},
{ onlyHash: true },
)
Copy link
Member

Choose a reason for hiding this comment

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

Why not sign the CID directly instead of adding this? Assuming it's the only thing we need to verify at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Signing CID bytes doesn't indicate the intent of the signature. It could be a number of things. This way makes it clear this is for the data availability proof.

src/services/new/fileUploadManger.ts Outdated Show resolved Hide resolved
@vaultec81 vaultec81 merged commit 818a1ad into vsc-eco:main Apr 3, 2024
1 check passed
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.

2 participants