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

feat: delegation.toJSON #186

Merged
merged 3 commits into from
Dec 15, 2022
Merged

feat: delegation.toJSON #186

merged 3 commits into from
Dec 15, 2022

Conversation

Gozala
Copy link
Collaborator

@Gozala Gozala commented Dec 14, 2022

This supersedes #173 and Fixes #172. It targets #185 as it relies on changes there.

In the other PR @alanshaw had this suggestion

If we get a fromJSON we can remove some code for storing delegations in the access Conf store.

And this PR still does not add fromJSON because, even though it will serialize the whole chain into a JSON which could be then de-serialized into compatible chain there is still a problem with the fact that fromJSON would not know if inline proofs do match claimed CIDs. This means that motivated attacker could use this an a attack vector.

In other words code that imports delegation should also validate hashes which is async operation and there for does not fit into a fromJSON signature.

That said I think it still useful to have toJSON method for debugging purposes, which is what this PR adds.

Copy link
Contributor

@gobengo gobengo left a comment

Choose a reason for hiding this comment

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

awesome

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

lgtm thanks @Gozala

@Gozala Gozala changed the base branch from fix/delegation-data-json to main December 15, 2022 22:41
@Gozala Gozala merged commit f8ffa74 into main Dec 15, 2022
This was referenced Apr 11, 2023
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.

Add toJSON method to delegations
4 participants