feat: API for uploading a sway project #19

sdankel · 2024-09-24T22:09:32Z

Closes #18

This PR adds a new API, upload_project, that accepts a .tgz file containing a sway project and does the following:

Strips out irrelevant files
Attempts to build the project
Uploads the ABI file IPFS and stores the hashes
Generates a bytecode ID from the compiled binary and stores it in a new postgres table
Creates a new .tgz from the cleaned up source code and uploads it to IPFS
Returns an upload ID, which can be used to publish metadata about the uploaded project (coming in a future PR)

Notes:

Because of the limitations of pinata and the pinata/IPFS rust SDKs, the source code is stored in IPFS as a .tgz file rather than as a uncompressed directory.
In order to build the sway project with the correct version of forc, we pre-install the latest 20 versions of forc on the docker image. As a fallback, the service uses cargo binstall to fetch the version of forc if it isn't pre-installed. This means the API can take a while to run if the version requested is not already installed.
I added one integration test for the happy path, but I didn't want to add more with different versions because the test takes a while to run. Happy to add more tests as needed.

vercel · 2024-09-24T22:09:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
forc-pub	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 21, 2024 5:29pm

JoshuaBatty · 2024-11-21T22:33:22Z

src/api/mod.rs

 }

 impl<'r, 'o: 'r> Responder<'r, 'o> for ApiError {
    fn respond_to(self, _request: &'r Request<'_>) -> rocket::response::Result<'o> {
+        println!("API error: {}", self);


should we use println_error from the forc-tracing crate here so our console output is standardised?

JoshuaBatty

Awesome work! just a couple of nits

JoshuaBatty · 2024-11-21T22:37:16Z

src/main.rs

+    fs::create_dir_all(forc_path.clone()).unwrap();
+    let forc_path = fs::canonicalize(forc_path.clone()).unwrap();
+
+    install_forc_at_path(forc_version, &forc_path)?;
+
+    // Create an upload ID and temporary directory.
+    let upload_id = Uuid::new_v4();
+    let upload_dir_str = format!("tmp/uploads/{}", upload_id);
+    let upload_dir = Path::new(&upload_dir_str);
+
+    fs::create_dir_all(upload_dir).unwrap();
+
+    // Persist the file to disk.
+    let orig_tarball_path = upload_dir.join("original.tgz");
+    tarball
+        .persist_to(&orig_tarball_path)
+        .await
+        .map_err(|_| ApiError::Upload(UploadError::SaveFile))?;
+
+    // Handle the project upload and store the metadata in the database.
+    let upload = handle_project_upload(
+        upload_dir,
+        &upload_id,
+        &orig_tarball_path,
+        &forc_path,
+        forc_version.to_string(),
+        pinata_client.inner(),
+    )
+    .await?;
+
+    let _ = db.conn().insert_upload(&upload)?;
+
+    // Clean up the temporary directory.
+    fs::remove_dir_all(upload_dir).unwrap();


Can we introduce an error type that we can use instead of unwrapping on the filesystem calls?

or change these to an expect?

JoshuaBatty · 2024-11-21T22:40:47Z

src/models.rs

+#[derive(Queryable, Selectable, Debug, Clone)]
+#[diesel(table_name = crate::schema::uploads)]
+#[diesel(check_for_backend(diesel::pg::Pg))]
+pub struct Upload {
+    pub id: Uuid,
+    pub source_code_ipfs_hash: String,
+    pub forc_version: String,
+    pub abi_ipfs_hash: Option<String>,
+    pub bytecode_identifier: Option<String>,
+    pub created_at: SystemTime,
+}
+
+#[derive(Insertable, Debug)]
+#[diesel(table_name = crate::schema::uploads)]
+pub struct NewUpload {
+    pub id: Uuid,
+    pub source_code_ipfs_hash: String,
+    pub forc_version: String,
+    pub abi_ipfs_hash: Option<String>,
+    pub bytecode_identifier: Option<String>,
+}


Maybe it's fine but my instinct here would be to try and only have 1 of these types considering they are duplicates apart from the final created_at field. Disregard if it doesn't make sense in the design though.

JoshuaBatty · 2024-11-21T22:41:54Z

src/pinata/mod.rs

+    }
+}
+
+pub struct MockPinataClient;


Maybe a comment here explaining the intended purpose of the Mock Client?

Also this is only used for testing; although minimal - this code would still get compiled into the release binary.
Hence maybe worthwhile creating and putting this within the test macro/mod or co-located with the tests.

JoshuaBatty · 2024-11-21T22:42:35Z

src/upload.rs

+    BytecodeId(String),
+}
+
+pub async fn handle_project_upload(


doc comment for this fn would be useful

JoshuaBatty · 2024-11-21T22:43:47Z

src/upload.rs

+    let project_dir = upload_dir.join("project");
+
+    // Unpack the tarball.
+    let tarball = File::open(orig_tarball_path).unwrap();


a few unwraps in the fn that could be turned into expects

zees-dev · 2024-11-22T02:04:51Z

migrations/2024-09-24-205353_uploads/up.sql

@@ -0,0 +1,8 @@
+CREATE TABLE uploads (


would help with replayability and iterating/testing:

Suggested change

CREATE TABLE uploads (

CREATE TABLE IF NOT EXISTS uploads (

zees-dev · 2024-11-22T02:09:00Z

migrations/2024-09-24-205353_uploads/down.sql

@@ -0,0 +1 @@
+DROP TABLE uploads


nit: could also do same here:

Suggested change

DROP TABLE uploads

DROP TABLE IF EXISTS uploads

zees-dev · 2024-11-22T02:09:59Z

src/api/publish.rs

+
+/// The response to an upload_project request.
+#[derive(Serialize, Debug)]
+pub struct UploadResponse {


nit?

Suggested change

pub struct UploadResponse {

pub struct PublishResponse {

zees-dev · 2024-11-22T02:14:15Z

src/pinata/mod.rs

+        dotenv().ok();
+        match (env::var("PINATA_API_KEY"), env::var("PINATA_API_SECRET")) {
+            (Ok(api_key), Ok(secret_api_key)) => {
+                let api = PinataApi::new(api_key, secret_api_key)
+                    .map_err(|_| UploadError::Authentication)?;
+                api.test_authentication()
+                    .await
+                    .map_err(|_| UploadError::Authentication)?;
+                Ok(PinataClientImpl { pinata_api: api })
+            }
+            _ => Err(UploadError::Ipfs),
+        }


nit: reduced nesting

Suggested change

dotenv().ok();

match (env::var("PINATA_API_KEY"), env::var("PINATA_API_SECRET")) {

(Ok(api_key), Ok(secret_api_key)) => {

let api = PinataApi::new(api_key, secret_api_key)

.map_err(|_| UploadError::Authentication)?;

api.test_authentication()

.await

.map_err(|_| UploadError::Authentication)?;

Ok(PinataClientImpl { pinata_api: api })

}

_ => Err(UploadError::Ipfs),

}

dotenv().ok();

let (api_key, secret_api_key) = match (env::var("PINATA_API_KEY"), env::var("PINATA_API_SECRET")) {

(Ok(key), Ok(secret)) => (key, secret),

_ => return Err(UploadError::Ipfs),

};

let api = PinataApi::new(api_key, secret_api_key)

.map_err(|_| UploadError::Authentication)?;

api.test_authentication()

.await

.map_err(|_| UploadError::Authentication)?;

Ok(PinataClientImpl { pinata_api: api })

I like both approaches 😅

With Sophie's it's compact which I'm a big fan of.

With Zeeshan's it's return early which I also a big fan of, but I've recently started trying to not leave variables dangling for the entire scope that aren't used (most of the time it's hard to do, but it's just a style that I've picked up lately).

Combining both approaches:

let api = match (env::var("PINATA_API_KEY"), env::var("PINATA_API_SECRET")) { (Ok(api_key), Ok(secret_api_key)) => { PinataApi::new(api_key, secret_api_key) .map_err(|_| UploadError::Authentication)? } _ => return Err(UploadError::Ipfs), }; api.test_authentication() .await .map_err(|_| UploadError::Authentication)?; Ok(PinataClientImpl { pinata_api: api })

zees-dev · 2024-11-22T02:24:59Z

src/main.rs

+    // Install the forc version if it's not already installed.
+    let forc_path_str = format!("forc-{forc_version}");
+    let forc_path = PathBuf::from(&forc_path_str);
+    fs::create_dir_all(forc_path.clone()).unwrap();


could also use the tempfile crate which handles creating temporary files and directories.
Not a fan of introducing external deps if not needed; but the good thing about the crate - and tempdir function is that if an error occurs anywhere in the function before the directory is removed, the directory would still be removed.
This is because the dir removal is based on the Drop trait impl - hence dir is removed as soon as the var goes out of scope.
^ this would ensure clean future runs of the function (assuming a previous run failed)

zees-dev · 2024-11-22T02:32:24Z

src/models.rs

+    pub forc_version: String,
+    pub abi_ipfs_hash: Option<String>,
+    pub bytecode_identifier: Option<String>,
+    pub created_at: SystemTime,


SystemTime does not support timezones in the DB and is generally less ergonomic than time::OffsetDateTime; for persistent data I'd reccomend TZ aware datetime primitive.

zees-dev · 2024-11-22T02:33:57Z

src/upload.rs

+    let unpacked_dir = upload_dir.join("unpacked");
+    let release_dir = unpacked_dir.join("out/release");
+    let project_dir = upload_dir.join("project");


nit: if you can foresee any re-usage of these strings; then worthwhile making these constants

zees-dev · 2024-11-22T02:34:39Z

src/upload.rs

+    archive.unpack(&unpacked_dir).unwrap();
+
+    // Remove `out` directory if it exists.
+    let _ = fs::remove_dir_all(unpacked_dir.join("out"));


nit: worthwhile using tempfile (based on previous comment)

zees-dev · 2024-11-22T02:43:12Z

src/upload.rs

+    .arg("--no-confirm")
+    .arg("--root")
+    .arg(forc_path)
+    .arg(format!("--pkg-url=https://github.com/FuelLabs/sway/releases/download/{forc_version}/forc-binaries-linux_arm64.tar.gz"))


May need to handle amd64 (x86) machine; you may need to match architectures beforehand

zees-dev · 2024-11-22T02:45:13Z

src/upload.rs

+}
+
+/// Installs the given version of forc at the specific root path using cargo-binstall.
+pub fn install_forc_at_path(forc_version: &str, forc_path: &Path) -> Result<(), UploadError> {


Instead of using the CLI; i'd reccomend using library/SDK functions to perform uploading operations.
They would be much lighter and ephemeral as one would not need to have the binary installed.
^ could be a future PR; just something to keep in mind

zees-dev · 2024-11-22T02:46:55Z

.env

would reccomend an .env.example file being committed in the repo; with the .env file ignored; and instructions in readme as quickstart:

cp .env.local .env

alfiedotwtf · 2024-11-22T20:51:00Z

src/main.rs

+    pinata_client: &State<PinataClientImpl>,
+    forc_version: &str,
+    mut tarball: TempFile<'_>,
+) -> ApiResult<UploadResponse> {


I'm wondering if forc_version will need to be sanitised e.g could a request escape the working directory?

/upload_project?v1../../../../../tmp

alfiedotwtf · 2024-11-22T22:07:51Z

Nice! Happy to approve once updated and you're happy for another review.

feat: API for file uploading

fd0ee50

vercel bot deployed to Preview September 24, 2024 22:10 View deployment

sdankel changed the title ~~feat: API for file uploading~~ feat: API for uploading a sway project Sep 25, 2024

install forc versions in docker

3dac88e

vercel bot deployed to Preview October 8, 2024 17:26 View deployment

clippy

c3db3ef

vercel bot deployed to Preview October 9, 2024 22:38 View deployment

generate bytecode identifier

2b7513a

vercel bot deployed to Preview November 21, 2024 13:57 View deployment

add forc bin test fixture

92f9240

vercel bot deployed to Preview November 21, 2024 14:14 View deployment

update test

12f29d4

vercel bot deployed to Preview November 21, 2024 15:27 View deployment

install forc in CI

6aac2ae

vercel bot deployed to Preview November 21, 2024 15:46 View deployment

update content type

9d3e9c1

vercel bot deployed to Preview November 21, 2024 17:29 View deployment

sdankel requested a review from a team November 21, 2024 17:32

sdankel marked this pull request as ready for review November 21, 2024 17:32

JoshuaBatty reviewed Nov 21, 2024

View reviewed changes

JoshuaBatty requested a review from a team November 22, 2024 01:37

JoshuaBatty assigned sdankel Nov 22, 2024

zees-dev reviewed Nov 22, 2024

View reviewed changes

alfiedotwtf reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: API for uploading a sway project #19

feat: API for uploading a sway project #19

sdankel commented Sep 24, 2024 •

edited

Loading

vercel bot commented Sep 24, 2024 •

edited

Loading

JoshuaBatty Nov 21, 2024

JoshuaBatty left a comment

JoshuaBatty Nov 21, 2024

JoshuaBatty Nov 21, 2024

JoshuaBatty Nov 21, 2024

JoshuaBatty Nov 21, 2024

zees-dev Nov 22, 2024

JoshuaBatty Nov 21, 2024

JoshuaBatty Nov 21, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

alfiedotwtf Nov 22, 2024 •

edited

Loading

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024

zees-dev Nov 22, 2024 •

edited

Loading

zees-dev Nov 22, 2024 •

edited

Loading

alfiedotwtf Nov 22, 2024

alfiedotwtf commented Nov 22, 2024

feat: API for uploading a sway project #19

Are you sure you want to change the base?

feat: API for uploading a sway project #19

Conversation

sdankel commented Sep 24, 2024 • edited Loading

vercel bot commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

JoshuaBatty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfiedotwtf Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zees-dev Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

zees-dev Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfiedotwtf commented Nov 22, 2024

sdankel commented Sep 24, 2024 •

edited

Loading

vercel bot commented Sep 24, 2024 •

edited

Loading

alfiedotwtf Nov 22, 2024 •

edited

Loading

zees-dev Nov 22, 2024 •

edited

Loading

zees-dev Nov 22, 2024 •

edited

Loading