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!: add inclusion claim to filecoin api #1307

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Feb 9, 2024

BREAKING CHANGE: aggregator events context need assert service

Apologies for the noise from some formatting changes, but this was from running pnpm format which was needed to make the CI happy 🤷🏼‍♂️

Similarly to how we issue piece/accept from insert event on inclusion-store, we can also use this event to issue inclusion/claim atomically. Therefore, we create a new event handler that w3filecoin-infra can wire up with a lambda from event on dynamo insert event

Closes: #1275

@vasco-santos vasco-santos force-pushed the feat/add-inclusion-claim-to-filecoin-api branch from fdc84d3 to 1663a25 Compare February 9, 2024 14:05
BREAKING CHANGE: aggregator events context need assert service
@vasco-santos vasco-santos force-pushed the feat/add-inclusion-claim-to-filecoin-api branch from 1663a25 to 4b28943 Compare February 9, 2024 14:08
@olizilla
Copy link
Contributor

Apologies for the noise from some formatting changes, but this was from running pnpm format which was needed to make the CI happy 🤷🏼‍♂️

in these cases it can help to have a commit in your branch that shows the clean diff with just the changes you want to make, and then a subsequent commit that applies just the formatting change that ci demands, so folks can review the meaningful stuff.

@vasco-santos
Copy link
Contributor Author

vasco-santos commented Feb 12, 2024

in these cases it can help to have a commit in your branch that shows the clean diff with just the changes you want to make, and then a subsequent commit that applies just the formatting change that ci demands, so folks can review the meaningful stuff.

Typically I would agree with you, if it would have failed in CI and I would fix.

However, that is not really the case here. I always run format before the commit because my own changes would be problematic for it... I could have looked at what files were purely changed by the format (which I didn't, but also would not expect commited things to be bad wrong!). So, the core changes from lint are actually in files I changed.

Anyway, this is not a big difference in the diff, so it would be needed considerable more time reverting specific changes from linting per file than eye balls ignoring the diff on these cases, which github already offers nice way to see that just formatted change. I think overall we have a problem with the linter and we should get rid of prettier because this only happens in this repo

nb: {
content: record.aggregate,
includes: record.piece,
proof: record.proof,
Copy link
Member

Choose a reason for hiding this comment

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

What is proof here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proof is the CID of the dag-cbor block that we encode the proof on for storing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Aggregator service should expose event handler to publish inclusion claim for each piece in an aggregate
3 participants