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] add txid to Emily withdrawals #1548

Merged
merged 6 commits into from
Mar 27, 2025
Merged

Conversation

MCJOHN974
Copy link
Contributor

@MCJOHN974 MCJOHN974 commented Mar 26, 2025

Description

Closes: #1529

Changes

  • added txid field for emily withdrawals table
  • added txid field for emily withdrawal api schemas

Testing Information

This PR adds pretty stupid thing -- just 1 more field to input jsons, output json, and to db. We already have integration tests checking that withdrawal we put to Emily equals to withdrawal we extract from Emily. Thus just adding new field to structs in this tests should test everything we need

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

@MCJOHN974 MCJOHN974 requested review from Jiloc, djordon and hozzjss March 26, 2025 10:03
@MCJOHN974
Copy link
Contributor Author

Hey, @hozzjss, is it true that new withdrawals are posted to Emily from bridge ?

Copy link
Contributor

@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.

Mostly done! I'd just make the field mandatory to keep future tech debt at minimum

This reverts commit 89f6716.
@MCJOHN974
Copy link
Contributor Author

I was thinking that we maybe want not Option here, thus I already have commit for not option

@MCJOHN974 MCJOHN974 marked this pull request as ready for review March 26, 2025 12:48
@MCJOHN974 MCJOHN974 requested a review from Jiloc March 26, 2025 13:06
@MCJOHN974
Copy link
Contributor Author

Thanks for your comments @Jiloc, addressed them in two commits above

Jiloc
Jiloc previously approved these changes Mar 26, 2025
Copy link
Contributor

@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.

lgtm!

@hozzjss
Copy link

hozzjss commented Mar 26, 2025

Thanks @MCJOHN974 but no withdrawals are not posted to emily from the bridge (THANK GOD)

@MCJOHN974
Copy link
Contributor Author

MCJOHN974 commented Mar 26, 2025

Thanks @MCJOHN974 but no withdrawals are not posted to emily from the bridge (THANK GOD)

Well, then from where they are pushed? I think we need update the pusher as well

upd: it is pushed from stacks node via sidecar, no action needed

@Jiloc
Copy link
Contributor

Jiloc commented Mar 26, 2025

Thanks @MCJOHN974 but no withdrawals are not posted to emily from the bridge (THANK GOD)

Well, then from where they are pushed? I think we need update the pusher as well

upd: it is pushed from stacks node via sidecar, no action needed

We discussed this at the standup but I am going to add here the answer for future reference.

The withdrawals events are sent by the sidecar to Emily via the new-block endpoint. The sidecar is connected to a stacks-node that emits a json message with those events, and just forward it them Emily. Emily parses it with the sbtc crate, and transforms it into events that process internally. So the withdrawal txid was already included in the messages, but just unused by Emily up until now

Copy link
Contributor

@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.

lgtm!

@MCJOHN974 MCJOHN974 requested a review from djordon March 27, 2025 11:47
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 ✅

@MCJOHN974 MCJOHN974 added this pull request to the merge queue Mar 27, 2025
Merged via the queue into main with commit 3c22232 Mar 27, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature]: Add transaction ID that created with withdrawal request in Emily responses
4 participants