Upload omnibus description to IPFS #15
Conversation
src/ipfs/index.spec.ts
Outdated
| }); | ||
|
|
||
| it("should throw an error if PINATA_TOKEN is not set", async () => { | ||
| sinon.stub(process, "env").value({ PINATA_TOKEN: undefined }); |
There was a problem hiding this comment.
Isn't the sinon.restore() call required there to remove the stub from the process.env object? Also, maybe it's better to add sinon.restore into the afterEach() hook.
src/ipfs/index.ts
Outdated
| import { uploadToPinata } from "./pinata"; | ||
|
|
||
| export async function uploadToIpfs(text: string, fileName: string) { | ||
| const pinataToken = process.env.PINATA_TOKEN; |
There was a problem hiding this comment.
The current abstraction appears incomplete.
If the goal is to support multiple providers, IPFSUploader should implement a strategy pattern, with Pinata as a concrete strategy. This approach is similar to the contract-info-resolver, where different strategies are used for the cache, such as in-memory or persistent. This configuration allows for the integration of new IPFS providers without requiring changes to the IPFSUploader interface.
Alternatively, if simplicity is the priority and there is no need for different providers, Pinata functionality could be directly incorporated into the uploadToIpfs() method. In this case, the Pinata token can be passed as a parameter and read from env by the calling method, simplifying testing and eliminating the need for a separate pinata.ts entity. However, adding a new IPFS provider would require modifications to the uploadToIpfs() method.
The current implementation does not allow for the addition of new IPFS providers without modifying the uploadToIpfs() method and adding a new entity for each new type of IPFS provider.
src/omnibuses/omnibus.ts
Outdated
| @@ -64,7 +72,15 @@ export class Omnibus<N extends NetworkName> { | |||
| } | |||
|
|
|||
| public get description(): string { | |||
| return this.titles.map((title, index) => `${index + 1}. ${title}`).join("\n"); | |||
| return this.roadmap.description || this.titles.map((title, index) => `${index + 1}. ${title}`).join("\n"); | |||
There was a problem hiding this comment.
titles and description should be two separate fields to enable validation of both, regardless of the description's presence in the omnibus.
Although the description is available at the omnibus launch, it might not be accessible to the bot that posts updates in Telegram chats. In such cases, the titles will serve as a fallback description for the omnibus. However, the correctness of the titles was not verified at the time of the omnibus launch.
| }); | ||
|
|
||
| after(() => { | ||
| mockUploadDescription.restore(); |
There was a problem hiding this comment.
Isn't the sinon.restore() enough here to restore both mocks?
|
|
||
| console.log(`Uploading the omnibus description to IPFS...`); | ||
| try { | ||
| omnibus.descriptionCID = await uploadDescription({ name: omnibus.name }, {} as any, {} as any); |
There was a problem hiding this comment.
We have an instance of the omnibus here, but we pass its name into the uploadDescription() method, which redundantly loads it from the file. This approach is inefficient and difficult to follow.
Additionally, uploadDescription() is typed as Hardhat's Action, yet it is not utilized as an action. If the aim is to reduce code duplication, the logic for uploading should be moved into a standalone method that can be used by both Hardhat's task and the checkDescription() method.
| omnibus.descriptionCID = await uploadDescription({ name: omnibus.name }, {} as any, {} as any); | ||
| } catch (e) { | ||
| console.error(`Failed to upload the omnibus description to IPFS: ${e}`); | ||
| const isConfirmed = await prompt.confirm( |
There was a problem hiding this comment.
It's better to move prompts to the task level
tasks/omnibuses.ts
Outdated
| console.log("\n"); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Does it make sense to check the description while the omnibus is launched on the fork?
…ep CID in tmp file
Ivan-Feofanov
force-pushed
the
feat/ipfs
branch
from
0a2696c to
3f3b3df
Compare
No description provided.