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.

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: Query By Recipient #1125

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

AshtonStephens
Copy link
Collaborator

@AshtonStephens AshtonStephens commented Dec 13, 2024

Description

Closes: #1122

Changes

This PR adds a new endpoint to emily. Unfortunately, the client autogeneration stopped working when I made the status optional in the get_deposits query, and I couldn't find any autogenerated solution, so to resolve this I've added a new endpoint:

https://{base_url}/deposit/recipient/{recipient}.

It works much like the other query but has the recipient in the path. This is a departure from the rest implementation, but it's the only way I could get it working.

In order to support this, there's now a new deposit table index that can be accessed by recipient.

Testing Information

Tested with an integration test in this PR.

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 force-pushed the query-deposits-by-recipient branch from 3e3ac32 to bf7315b Compare December 19, 2024 13:45
@AshtonStephens AshtonStephens requested a review from Jiloc December 19, 2024 14:07
@AshtonStephens AshtonStephens self-assigned this Dec 19, 2024
@AshtonStephens AshtonStephens added the emily API that communicates with Signers to trigger sBTC operations. label Dec 19, 2024
@AshtonStephens AshtonStephens marked this pull request as ready for review December 19, 2024 14:10
@djordon djordon added this to the sBTC: Nice to have milestone Dec 19, 2024
@AshtonStephens AshtonStephens changed the title feat: Query By Recipient without tests feat: Query By Recipient Dec 20, 2024
pub next_token: Option<String>,
/// Maximum number of results to show.
#[serde(skip_serializing_if = "Option::is_none")]
pub page_size: Option<i32>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why page_size is i32 and not u32? From "Maximum number of results to show." description I expect that negative values don't make any sense

@@ -314,7 +324,7 @@ async fn get_deposits() {
// If the number of elements is an exact multiple of the chunk size the "final"
// query will still have a next token, and the next query will now have a next
// token and will return no additional data.
let expected_chunks = expected_deposit_infos.len() as i32 / chunksize + 1;
let expected_chunks: i32 = expected_deposit_infos.len() as i32 / chunksize + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here as well, should it be u32 instead i32?

}

// The size of the chunks to grab from the api.
let chunksize: i32 = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here also probably u32 can work

Copy link
Collaborator

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

Given that we are not rushing this feature, would it make sense to have another time-gated effort to try it to make it work with the get_deposits endpoint as a query? If you think that's just time wasted, I am fine with going forward as it is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emily API that communicates with Signers to trigger sBTC operations.
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

[Feature]: Add a way to filter deposits by recipient in Emily
4 participants