-
Notifications
You must be signed in to change notification settings - Fork 0
fix: update IPFS tasks to use environment variables from .env files #118
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
Conversation
Reviewer's Guide by SourceryThe pull request refactors IPFS task scripts to utilize environment variables from .env files, removing the need for dynamic environment fetching and simplifying the configuration process. Class diagram for IPFS task refactoringclassDiagram
class IPFSTask {
+pinToNFTStorage(blob: Blob)
+ipfsUpload(content: Buffer | Record<string, any>, pin?: true)
}
class IPFSCIDTask {
+setAction(ipfspath: string)
}
class Environment {
+nftStorageToken: String
+btpIpfs: String
+settlemintAccessToken: String
}
IPFSTask --> Environment : uses
IPFSCIDTask --> Environment : uses
note for Environment "Environment variables are now loaded from .env files"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SaeeDawod - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Review instructions: 3 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
// Create IPFS client with the API endpoint | ||
const ipfsClient = create({ | ||
url: baseUrl, | ||
url: btpIpfs, | ||
headers: { | ||
'x-auth-token': process.env.BTP_SERVICE_TOKEN!, | ||
'x-auth-token': process.env.SETTLEMINT_ACCESS_TOKEN!, | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Validate the presence of SETTLEMINT_ACCESS_TOKEN
Since this token is critical for creating a valid IPFS client, consider adding a check to ensure that SETTLEMINT_ACCESS_TOKEN is set to prevent potential runtime issues.
// Create IPFS client with the API endpoint | |
const ipfsClient = create({ | |
url: baseUrl, | |
url: btpIpfs, | |
headers: { | |
'x-auth-token': process.env.BTP_SERVICE_TOKEN!, | |
'x-auth-token': process.env.SETTLEMINT_ACCESS_TOKEN!, | |
}, | |
}); | |
// Validate that SETTLEMINT_ACCESS_TOKEN environment variable is set | |
const settlemintAccessToken = process.env.SETTLEMINT_ACCESS_TOKEN; | |
if (!settlemintAccessToken) { | |
throw new Error("Environment variable SETTLEMINT_ACCESS_TOKEN is not set."); | |
} | |
// Create IPFS client with the API endpoint | |
const ipfsClient = create({ | |
url: btpIpfs, | |
headers: { | |
'x-auth-token': settlemintAccessToken, | |
}, | |
}); |
import { nftStorageToken } from '../hardhat.config'; | ||
|
||
// NFT Storage token - ideally this would be in config or .env | ||
const nftStorageToken = process.env.NFT_STORAGE_TOKEN || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (review_instructions): Using an empty string as a fallback for the NFT storage token could lead to silent failures.
If the token is missing, the code will continue execution with an empty string and only fail later when trying to use it. Consider throwing an error immediately if the environment variable is not set to prevent potential issues downstream.
Review instructions:
Path patterns: **/*.ts
Instructions:
Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
headers: { | ||
'x-auth-token': process.env.BTP_SERVICE_TOKEN!, | ||
'x-auth-token': process.env.SETTLEMINT_ACCESS_TOKEN!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (review_instructions): Using the non-null assertion operator (!) could lead to runtime errors if the token is missing.
The code assumes SETTLEMINT_ACCESS_TOKEN will always be defined. If this environment variable is missing, this will cause a runtime error. Consider adding a check for this token's existence before creating the IPFS client, similar to how you check for the IPFS endpoint.
Review instructions:
Path patterns: **/*.ts
Instructions:
Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
headers: { | ||
'x-auth-token': process.env.BTP_SERVICE_TOKEN!, | ||
'x-auth-token': process.env.SETTLEMINT_ACCESS_TOKEN!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (review_instructions): Using the non-null assertion operator (!) could lead to runtime errors if the token is missing.
The code assumes SETTLEMINT_ACCESS_TOKEN will always be defined. If this environment variable is missing, this will cause a runtime error. Consider adding a check for this token's existence before creating the IPFS client, similar to how you check for the IPFS endpoint.
Review instructions:
Path patterns: **/*.ts
Instructions:
Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
📦 Packages
|
Summary by Sourcery
Update IPFS tasks to utilize environment variables from .env files, removing the need to fetch them from an external service, and simplify the IPFS client creation process.
Bug Fixes:
Enhancements: