-
Notifications
You must be signed in to change notification settings - Fork 14
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
Emily Rust Lambda Boilerplate #101
Conversation
71456d6
to
de0f5b3
Compare
de0f5b3
to
4c86ea5
Compare
33c2da9
to
1b085e5
Compare
#[test_case(Method::GET, "/chainstate/{height}", None; "get-chainstate")] | ||
#[test_case(Method::PUT, "/chainstate/{height}", None; "set-chainstate")] | ||
#[test_case(Method::POST, "/chainstate/{height}", None; "update-chainstate")] | ||
#[tokio::test] |
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 rather have the test_case
traits defined after tokio::test
but unfortunately that's not possible. Not a big problem ~
emily = { version = "0.1.0", path = ".generated-sources/emily" } | ||
tokio = "1.32.0" | ||
serde = "1.0" | ||
serde_json = "1.0" | ||
lambda_runtime = "0.11.1" | ||
aws_lambda_events = "0.15.0" | ||
http = "1.1.0" | ||
aws-sdk-dynamodb = { version = "1.3.0" } | ||
futures = "0.3.24" | ||
# This is necessary to compile the AWS Lambda as a lambda. | ||
openssl = { version = "0.10", features = ["vendored"] } |
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 I be updating these in the workspace root, or on a package basis?
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'm okay with either, but I think workspace root makes the most sense.
As an aside, are you sure that openssl is necessary for AWS Lambda? As in, why wouldn't rustls work? I think this dependency is fine, since having it vendored makes potential compilation issues go away (I think), I just recall having issues with openssl when building with MUSL (might have been my fault, but still).
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 bit is predictive of the next PR, I can remove it for this one. It's not great practice to include things needed for the next PR.
This is what it's needed for:
As part of that PR we need to run this command to compile the .zip
file that a is unpacked and run in the lambda.
cargo lambda build --release --output-format zip [--arm64]
The Cargo Lambda command uses shared object files during compilation, and MacOS uses the sources in the directory marked by the environment variable OPENSSL_DIR
. The problem is mac uses files like libcrypto.3.dylib
instead of libcrypto.3.so
which is what the compiler needs.
After looking through a bunch of solutions this was the only thing that seemed to work across platforms. I also wonder if there was a better solution but this looks to be the standard one.
|
||
// Assert. | ||
assert_eq!(response.status_code, 201); | ||
assert_eq!(response.body.unwrap(), Body::Text("{\"bitcoinTxid\":\"BITCOIN_TXID_UNITTEST\",\"bitcoinTxOutputIndex\":0.0,\"recipient\":\"MOCK_RECIPIENT\",\"amount\":11111.0,\"status\":\"PENDING\",\"statusMessage\":\"MOCK_CREATE_DEPOSIT_RESPONSE\",\"parameters\":{\"maxFee\":33333.0,\"lockTime\":22222.0,\"reclaimScript\":\"MOCK_RECLAIM_SCRIPT\"}}".to_string())); |
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 know this is bad, this is just for the boilerplate.
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.
Maybe it's 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.
I think it's fine, especially so early in development.
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.
As initial boilerplate, this looks fine. I did have a question about some of the generated models though.
emily = { version = "0.1.0", path = ".generated-sources/emily" } | ||
tokio = "1.32.0" | ||
serde = "1.0" | ||
serde_json = "1.0" | ||
lambda_runtime = "0.11.1" | ||
aws_lambda_events = "0.15.0" | ||
http = "1.1.0" | ||
aws-sdk-dynamodb = { version = "1.3.0" } | ||
futures = "0.3.24" | ||
# This is necessary to compile the AWS Lambda as a lambda. | ||
openssl = { version = "0.10", features = ["vendored"] } |
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'm okay with either, but I think workspace root makes the most sense.
As an aside, are you sure that openssl is necessary for AWS Lambda? As in, why wouldn't rustls work? I think this dependency is fine, since having it vendored makes potential compilation issues go away (I think), I just recall having issues with openssl when building with MUSL (might have been my fault, but still).
status: emily::models::OpStatus::Pending, | ||
status_message: "MOCK_CREATE_DEPOSIT_RESPONSE".to_string(), | ||
parameters: Box::new(DepositParameters { | ||
lock_time: Some(22222.0), |
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.
Wow, that's a long time.
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.
Wait why is amount
, lock_time
, and max_fee
not an integers?
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's just what the autogenerator provided, but I am not sure why it does that, the template specifically says Integer
.
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 don't know how, but I bet this has to do with JavaScript using the same "number" type for anything numeric 😅.
|
||
// Assert. | ||
assert_eq!(response.status_code, 201); | ||
assert_eq!(response.body.unwrap(), Body::Text("{\"bitcoinTxid\":\"BITCOIN_TXID_UNITTEST\",\"bitcoinTxOutputIndex\":0.0,\"recipient\":\"MOCK_RECIPIENT\",\"amount\":11111.0,\"status\":\"PENDING\",\"statusMessage\":\"MOCK_CREATE_DEPOSIT_RESPONSE\",\"parameters\":{\"maxFee\":33333.0,\"lockTime\":22222.0,\"reclaimScript\":\"MOCK_RECLAIM_SCRIPT\"}}".to_string())); |
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 it's fine, especially so early in development.
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 stuff. I'd just like the TODOs to be addressed or labeled before seeing this merged.
Description
Creates a Rust boilerplate lambda that responds to the requests from the
emily
api spec with responses of the right501
error format.Build Annoyances
The autogenerated client makes updates to the API definition trivial as you can see with this update, but I'm not convinced the autobuilding setup with
build.rs
that I included in #99 is the whole solution.How can we have the autogenerated client not cause
cargo build
to fail unlesscargo build --package emily
is run first?Testing