Skip to content
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.

Sign up for GitHub

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

Jump to bottom

595 feature provide raw transaction data for emily api deposit operations #601

Merged
merged 5 commits into from
Oct 3, 2024
Merged

595 feature provide raw transaction data for emily api deposit operations #601

merged 5 commits into from
Oct 3, 2024

Conversation

AshtonStephens
Copy link
Collaborator

@AshtonStephens AshtonStephens commented Oct 2, 2024 •
edited
Loading

Description

Closes: #595

Changes

  1. Adds a feature to the Emily API CDK template where the AWS resources can be created without the ApiGateway and the Emily Lambda when an environment variable is set. This is useful for testing when the Emily lambda handler isn't going to be fielding the API requests
  2. Adds deposit_script and reclaim_script to the Emily API Deposit and DepositInfo resources and updates tests accordingly.

Testing Information

Tested with integration tests. At the moment this requires three separate commands to run concurrently, and is easiest with three terminals.

  • Terminal 1: make emily-integration-env-up
  • Terminal 2: make emily-server-watch
  • Terminal 3: make emily-integration-test

Note: We won't be fixing this need for three terminals for integration tests soon so we can focus solely on the core product flows of handling deposits and withdrawals, but the ticket #505 tracks the need to do it.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@AshtonStephens AshtonStephens added the emily API that communicates with Signers to trigger sBTC operations. label Oct 2, 2024
@AshtonStephens AshtonStephens self-assigned this Oct 2, 2024
@AshtonStephens AshtonStephens linked an issue Oct 2, 2024 that may be closed by this pull request
[Feature]: Provide raw transaction data for Emily API deposit operations #595
Closed
1 task
@AshtonStephens AshtonStephens marked this pull request as ready for review October 2, 2024 11:55
@AshtonStephens AshtonStephens requested review from djordon and removed request for cylewitruk October 2, 2024 11:55
matteojug
matteojug reviewed Oct 2, 2024
View reviewed changes
emily/handler/src/api/models/deposit/mod.rs Outdated Show resolved Hide resolved
emily/handler/src/api/models/deposit/mod.rs Outdated
Comment on lines 52 to 55
/// Raw reclaim script binary.
pub reclaim_script: String,
/// Raw deposit script binary.
pub deposit_script: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: I was looking around and noticed that those scripts, as well as things like bitcoin_txid, are Strings; and I didn't see any reference to specific encodings used (eg, hex or b64). So, if those are just normal strings, don't we risk to break everything if the raw binary is not utf8-friendly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this isn't specified in the spec either. Can we make this a hex string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only be a comment change for now since these aren't being messed with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed the comment to the following:

    /// Raw reclaim script binary in hex.
    pub reclaim_script: String,
    /// Raw deposit script binary in hex.
    pub deposit_script: String,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cool see. I guess that means that we require the input be a hex string. Should we clarify that in an addendum to the spec, or just update it as is now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update as it is now

AshtonStephens
AshtonStephens commented Oct 3, 2024
View reviewed changes
emily/cdk/lib/emily-stack.ts
Comment on lines +48 to +62
if (!EmilyStackUtils.isTablesOnly()) {
const operationLambda: lambda.Function = this.createOrUpdateOperationLambda(
depositTableName,
withdrawalTableName,
chainstateTableName,
props
);

// Give the operation lambda full access to the DynamoDB tables.
depositTable.grantReadWriteData(operationLambda);
withdrawalTable.grantReadWriteData(operationLambda);
chainstateTable.grantReadWriteData(operationLambda);
// Give the operation lambda full access to the DynamoDB tables.
depositTable.grantReadWriteData(operationLambda);
withdrawalTable.grantReadWriteData(operationLambda);
chainstateTable.grantReadWriteData(operationLambda);

const emilyApi: apig.SpecRestApi = this.createOrUpdateApi(operationLambda, props);
const emilyApi: apig.SpecRestApi = this.createOrUpdateApi(operationLambda, props);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevents the missing bootstrap.zip case when running the makefile command.

djordon
djordon approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ✅

@AshtonStephens AshtonStephens merged commit 0241c7e into main Oct 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteojug matteojug matteojug approved these changes

@djordon djordon djordon approved these changes

Assignees

@AshtonStephens AshtonStephens

Labels
emily API that communicates with Signers to trigger sBTC operations.
Projects
sBTC
Archived in project
Milestone
Minimum Working Deposit Flow
Development

Successfully merging this pull request may close these issues.

[Feature]: Provide raw transaction data for Emily API deposit operations
3 participants
@AshtonStephens @matteojug @djordon