-
Notifications
You must be signed in to change notification settings - Fork 23
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 dag_to_car functionality #1622
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: d4v1d03 <[email protected]>
Signed-off-by: Amit Pandey <[email protected]>
Signed-off-by: Amit Pandey <[email protected]>
Signed-off-by: Amit Pandey <[email protected]>
Signed-off-by: Amit Pandey <[email protected]>
packages/w3up-python-client/setup.py
Outdated
print("\n⚠️ Some dependencies could not be installed automatically.") | ||
print("Please install them manually before using the package.") | ||
print("- ipfs-car: npm install -g ipfs-car") | ||
print("- w3cli: npm install -g @web3-storage/w3") |
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.
print("- w3cli: npm install -g @web3-storage/w3") | |
print("- w3cli: npm install -g @web3-storage/w3cli") |
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.
Fixed
@@ -0,0 +1,154 @@ | |||
"""import os |
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.
Is this meant to be commented?
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.
my bad, i forgot to remove this file
|
||
auth_secret = os.getenv("X-AUTH-SECRET-HEADER") | ||
authorization = os.getenv("AUTHORIZATION-HEADER") | ||
endpoint = os.getenv("HTTPS-ENDPOINT", "https://up.storacha.network/bridge") |
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.
Env vars should use underscore (_
) not dash (-
) to be a valid bash 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.
Fixed
return response.json() | ||
else: | ||
print(f"❌ Upload failed with status code {response.status_code}: {response.text}") | ||
return None |
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.
Why not raise an error here?
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.
Fixed, please have a look
json.dump(payload, temp, indent=2) | ||
temp_json_path = temp.name | ||
|
||
return temp_json_path |
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.
Why does it store these and not return them?
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.
Fixed
|
||
def upload_to_storacha(json_file): | ||
"""Upload to Storacha using the HTTP Bridge.""" | ||
load_dotenv() |
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.
I would pass these dotenv vars to the function as parameters, and load them in main()
. This allows the script to be used as a library where the values could come from places oter than environment vars.
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.
Fixed, please have a look
|
||
cid, car_file = create_dag_and_car(file_path) | ||
|
||
json_file = create_all_instructions_json(cid, space_did) |
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.
Why all "instructions"? We only want to do store/add
next...
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.
Fixed this, I've put other instructions that the user can use in the README.md
return cid, car_file | ||
except subprocess.CalledProcessError as e: | ||
print(f"❌ ipfs-car failed: {e.stderr.decode('utf-8')}") | ||
sys.exit(1) |
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.
I'd avoid print
ing and sys.exit
in methods that could be used as library code. Instead catch the error here, and re-throw it with the constructed error message. It can be caught in main()
if you like but I assume a raised error is going to be printed to the console and the exit code set to non-zero anyways.
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.
Fixed
return None | ||
|
||
def create_dag_and_car(file_path): | ||
if not os.path.exists(file_path): |
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.
ipfs-car
will do this - I think this is redundant.
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.
fixed
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.
It is? I still see the conditional here.
Signed-off-by: Amit Pandey <[email protected]>
Signed-off-by: Amit Pandey <[email protected]>
|
||
## Instructions to be used over the http bridge | ||
|
||
Below instructions can be used over the http bridge by adidng them to the list.json file. |
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.
Below instructions can be used over the http bridge by adidng them to the list.json file. | |
Below instructions can be used over the http bridge by adding them to the list.json file. |
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.
Thanks, fixed the changes.
] | ||
*/ | ||
] | ||
} |
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.
I think this is a bit confusing, why not expose these as separate methods on the client?
return None | ||
|
||
def create_dag_and_car(file_path): | ||
if not os.path.exists(file_path): |
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.
It is? I still see the conditional here.
raise FileNotFoundError(f"File '{file_path}' not found.") | ||
|
||
car_file = f"{file_path}.car" | ||
ipfs_car_cmd = f"npx ipfs-car pack {file_path} --output {car_file}" if platform.system() == "Windows" else f"ipfs-car pack {file_path} --output {car_file}" |
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.
Why npx only on windows?
Why not install ipfs-car
as part of the setup?
print("❌ w3cli not found.") | ||
print("📦 Installing w3cli globally...") | ||
try: | ||
subprocess.run("npm install -g @web3-storage/w3cli", shell=True, check=True) |
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.
Does it need to be global - can you just add it to the package.json
here and install and use it locally?
payload = f.read() | ||
return payload | ||
|
||
def upload_to_storacha(json_payload, auth_secret, authorization, endpoint): |
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.
I'd perhaps rename - this is "send request to the bridge".
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.
Fixed
try: | ||
cid, car_file = create_dag_and_car(file_path) | ||
json_payload = read_list_json() | ||
result = upload_to_storacha(json_payload, auth_secret, authorization, endpoint) |
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.
I think we're missing a step here. The response from the bridge will have the URL for where you should HTTP PUT the data. Have a read through https://docs.storacha.network/how-to/http-bridge/#storing-files
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.
My bad, i checked this right now. Fixed it now
Signed-off-by: Amit Pandey <[email protected]>
Draft PR on the issue #1621