Skip to content

Conversation

@volmedo
Copy link
Member

@volmedo volmedo commented Nov 18, 2025

These changes require storacha/upload-service#570 to land first.

Handle STOREFRONT_PROOF as a secret rather than an environment variable.

Also, I noticed that some of the functions that were referencing the proof were not actually having it passed when defined in the stack, so I bound the secret to their configs.

Finally, the STOREFRONT_PROOF was renamed to AGGREGATOR_SERVICE_PROOF, as that aligns with what it actually is. I also removed the storefront proof from everywhere, based on the assumption that the storefront service will always be the upload service. Thus, invocations to the storefront service (the filecoin/* family of capabilities) are self-signed by the upload service and require no proofs.

@seed-deploy
Copy link

seed-deploy bot commented Nov 18, 2025

View stack outputs

let id = getServiceSigner({
privateKey
})
const storefrontServiceProofs = []
Copy link
Member Author

Choose a reason for hiding this comment

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

the proof is parsed but then it's not used anywhere. I guess it should go in the context object below?

Copy link
Member

Choose a reason for hiding this comment

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

😩 yeah it's going to require changes in storefrontEvents.handleCronTick to accept the proof. It needs the proof so that effect it creates has the correct CID.

Copy link
Member

Choose a reason for hiding this comment

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

We should also do id = id.withDID(DID.parse(did).did()) regardless.

Copy link
Member Author

Choose a reason for hiding this comment

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

😩 yeah it's going to require changes in storefrontEvents.handleCronTick to accept the proof. It needs the proof so that effect it creates has the correct CID.

I just understood this comment 😞

Comment on lines 49 to 50
let storefrontSigner = getServiceSigner({
privateKey
})
Copy link
Member Author

Choose a reason for hiding this comment

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

is this correct? Shouldn't this have the upload service's DID if we are using a proof?

Comment on lines 68 to 62
issuer: storefrontSigner,
with: storefrontSigner.did(),
audience: storefrontSigner,
Copy link
Member Author

Choose a reason for hiding this comment

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

oh, ok, so I guess we need both the aggregator service's DID and the upload service's DID to fix this invocation config

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the proof should be a proof for the uplaod service to use the aggregator and the aggregator DID needs to be the audience.

Copy link
Member

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM by me

Base automatically changed from ash/fix/proof-parsing to main November 19, 2025 21:23
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

From a consistency POV I like this, but really the only reason the other proofs are secrets is because of the environment variable size limit which we are close to with the ucan-invocation-router.js lambda. We are not anywhere near that limit for these other lambdas so it is not really necessary.

If we do this, I would rather that we combine it with renaming the variable more appropriately. It should be named after the service it is used with, not the service that is using it (otherwise all proofs would be "storefront"/"upload service"!). This is already confused in the code here.

Unfortunately there is a bunch more work here than just moving an environment variable to secrets.

let id = getServiceSigner({
privateKey
})
const storefrontServiceProofs = []
Copy link
Member

Choose a reason for hiding this comment

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

😩 yeah it's going to require changes in storefrontEvents.handleCronTick to accept the proof. It needs the proof so that effect it creates has the correct CID.

let id = getServiceSigner({
privateKey
})
const storefrontServiceProofs = []
Copy link
Member

Choose a reason for hiding this comment

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

We should also do id = id.withDID(DID.parse(did).did()) regardless.

Comment on lines 68 to 62
issuer: storefrontSigner,
with: storefrontSigner.did(),
audience: storefrontSigner,
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the proof should be a proof for the uplaod service to use the aggregator and the aggregator DID needs to be the audience.

@volmedo
Copy link
Member Author

volmedo commented Nov 27, 2025

If we do this, I would rather that we combine it with renaming the variable more appropriately. It should be named after the service it is used with, not the service that is using it (otherwise all proofs would be "storefront"/"upload service"!). This is already confused in the code here.

Unfortunately there is a bunch more work here than just moving an environment variable to secrets.

It wasn't clear to me that storefront was actually referring to the upload-service 🤷🏻

I'm working on this.

@volmedo
Copy link
Member Author

volmedo commented Dec 3, 2025

@alanshaw I renamed STOREFRONT_PROOF to AGGREGATOR_SERVICE_PROOF and wired it where needed. The rest of the work required to pass the aggregator proof down is in storacha/upload-service#570.

There were also some functions getting a storefront proof, which allows using a storefront service that is not the upload service. I removed this possibility for now to simplify the code because we are not using it today. If you think we need that flexibility, let me know and I'll implement it properly.

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.

4 participants