-
Notifications
You must be signed in to change notification settings - Fork 43
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: backend for discussion #88
Conversation
@tensojka please take a look at what I've done so far. I haven't tested it yet though. I still need to implement the function that resolves starknet address to starknet.id and setup pinata to test it |
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.
Looks fine so far, but I'm not sure whether Helia can only compute the CID without spawning a full node.
backend/src/ipfs.ts
Outdated
let helia: any; | ||
|
||
export const initializeHelia = async () => { | ||
helia = await createHeliaHTTP(); |
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.
Are you sure that Helia does not initialize a full node (one that connects to DHT)? We don't want that
Whenever you want another review, re-request review from me please. Thanks |
@tensojka I've made some changes to the code based on your review. The only thing not working yet is the resolution of starknet.id from address so I commented out the logic for that in |
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.
Have you tested it? Does Helia give the same hash as IPFS? Is it really async and does it throw an error if it fails?
backend/src/ipfs.ts
Outdated
import { getStarknetId } from "./starknet.js"; | ||
|
||
const PINATA_JWT = | ||
"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VySW5mb3JtYXRpb24iOnsiaWQiOiJmYjMxZmY2Ny0wNGZmLTQ2YWYtOTc1NC00MDZiNTQ5MDhlOWYiLCJlbWFpbCI6ImVqZW1iaW9jaGU1MEBnbWFpbC5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwicGluX3BvbGljeSI6eyJyZWdpb25zIjpbeyJpZCI6IkZSQTEiLCJkZXNpcmVkUmVwbGljYXRpb25Db3VudCI6MX0seyJpZCI6Ik5ZQzEiLCJkZXNpcmVkUmVwbGljYXRpb25Db3VudCI6MX1dLCJ2ZXJzaW9uIjoxfSwibWZhX2VuYWJsZWQiOmZhbHNlLCJzdGF0dXMiOiJBQ1RJVkUifSwiYXV0aGVudGljYXRpb25UeXBlIjoic2NvcGVkS2V5Iiwic2NvcGVkS2V5S2V5IjoiMjQ1ZDhkMmExMjg1ZWM1YWZlNjAiLCJzY29wZWRLZXlTZWNyZXQiOiI3OWI1NWM3MzZkZjBkODI2MjNiOWMxZDBhMzkxMjNlOTljNjhiNjE0ZDgwZGI2ZWI5OGM0MzIzZWM3MzI1OWUzIiwiaWF0IjoxNzE3MTAxNjU2fQ.N3pHXlmJsN0HZpkHUp2RMYF49C90WmwzNBTSjLxNzNc"; |
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.
A bot just read this JWT and it's compromised. You must load it from an environment variable.
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.
Don't forget to revoke this JWT and generate a new one.
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.
A bot just read this JWT and it's compromised. You must load it from an environment variable.
Yea, it was careless mistake by me. I'll fix it
@tensojka I have tested it out locally and can confirm that all functionalities are operating as expected. However, please note that Pinata requires a premium subscription for pinning by hash. As such, I was unable to fully verify this specific feature. The attached screenshot provides additional context on this matter. |
You can't use pinning by hash – pinning by hash works by Pinata fetching the file according to the hash from IPFS, but we don't want to operate a IPFS node that would serve the file to Pinata. Therefore, you must send the file to Pinata and have them generate the hash. (And check that their hash is the same as the one sent to the client) |
Oh okay. Got it! |
@tensojka I effected the changes you requested. The screenshot below shows that result I got from testing. |
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.
Great, I was able to load it from IPFS!
backend/src/ipfs.ts
Outdated
export const submitProposal = async (req: Request, res: Response) => { | ||
const { text, address } = req.body; | ||
|
||
if (!text || !address) { | ||
return res.status(400).json({ error: "Missing text or address" }); | ||
} | ||
|
||
const proposalData: Proposal = { | ||
text, | ||
address | ||
}; |
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.
You should add assertions about the address and possibly also the text (max length) so this can't be used to store arbitrary data.
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.
Alright
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.
What max length do you think is convenient?
I will get it working tomorrow 👌 |
backend/src/starknet.ts
Outdated
address: string | ||
): Promise<string | null> => { | ||
const provider = new RpcProvider({ | ||
nodeUrl: "your_mainnet_node_url", |
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.
Use our node URL from Scarb.toml – http://34.22.208.73:6060/v0_7
Or better, load it from an environment variable
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.
Okay
deployed at |
- add type assertions and validations to `text` and `address`
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.
Otherwise lgtm!
backend/src/ipfs.ts
Outdated
starknet_id?: string; | ||
} | ||
|
||
const ADDRES_REGEX: RegExp = /^0x0[0-9a-fA-F]{63}$/; |
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.
this will match only addresses starting with 0x0 no?
Address can be even 0x1234
Also, not necessarily 63
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.
Alright, will fix it right away
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.
should be 64 actually, 0x is two, 64 is the rest
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.
okay
backend/src/ipfs.ts
Outdated
} | ||
|
||
const ADDRES_REGEX: RegExp = /^0x0[0-9a-fA-F]{63}$/; | ||
const MAX_LENGTH: number = 200; |
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.
10k should be fine, no reason to constrain it a lot
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.
Okay
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.
LGTM, thanks!
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.
Bug still not fixed :( 👎
No description provided.