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(zcoin): impl balance event streaming #2076

Merged
merged 13 commits into from
Mar 29, 2024
Merged

feat(zcoin): impl balance event streaming #2076

merged 13 commits into from
Mar 29, 2024

Conversation

borngraced
Copy link
Member

@borngraced borngraced commented Mar 1, 2024

Implement balance event streaming for zcoin for Native and WASM target

After each update to the wallet database with a new block, we check for any transactions within the block. If transactions are detected, we initiate a balance event notification to retrieve the latest balance from the database and send/update

@borngraced borngraced added the in progress Changes will be made from the author label Mar 1, 2024
@borngraced borngraced self-assigned this Mar 1, 2024
@borngraced borngraced added under review and removed in progress Changes will be made from the author labels Mar 3, 2024
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

Some notes from my end:

Comment on lines 1000 to 1003
let (z_balance_event_sender, z_balance_event_handler) = match get_z_balance_event_handlers(self.ctx) {
Some((sender, receiver)) => (Some(sender), Some(receiver)),
None => (None, None),
};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You can handle ctx.event_stream_configuration directly here without needing to return Option in get_z_balance_event_handlers, or even better remove get_z_balance_event_handlers entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

done thanks! :)

Comment on lines 18 to 20
pub enum ZBalanceEvent {
Triggered,
}
Copy link
Member

Choose a reason for hiding this comment

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

if it's going to be only this "Triggered" value for event handlers, you can use () instead of an enum type.

Copy link
Member Author

Choose a reason for hiding this comment

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

done thanks!

Comment on lines 72 to 73
while let Some(event) = bal.next().await {
match event {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to match the event

Copy link
Member Author

Choose a reason for hiding this comment

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

done, thanks

onur-ozkan
onur-ozkan previously approved these changes Mar 25, 2024
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. We can merge this once the @KomodoPlatform/qa team confirms this is working as expected on Native and WASM platforms.

Comment on lines +23 to +24
const EVENT_NAME: &'static str = "COIN_BALANCE";
const ERROR_EVENT_NAME: &'static str = "COIN_BALANCE_ERROR";
Copy link
Member

Choose a reason for hiding this comment

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

Note for the future development: We may need to maintain all the available event names from a single source without needing to type them manually for each implementation.

mariocynicys
mariocynicys previously approved these changes Mar 29, 2024
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Just one nit, LGTM otherwise.

dex_fee_addr,
my_z_addr,
my_z_addr_encoded,
evk: ExtendedFullViewingKey::from(&z_spending_key),
z_spending_key,
z_tx_prover: Arc::new(z_tx_prover),
light_wallet_db,
consensus_params: self.protocol_info.consensus_params,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this clone is redundant? no?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, thanks.

@borngraced borngraced dismissed stale reviews from mariocynicys and onur-ozkan via 498880d March 29, 2024 13:45
@shamardy
Copy link
Collaborator

@borngraced please resolve conflicts that appeared due to merging the tx history PR.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Great work!

@shamardy shamardy merged commit a81a67f into dev Mar 29, 2024
28 of 34 checks passed
@shamardy shamardy deleted the z_event_streaming branch March 29, 2024 20:05
dimxy added a commit to dimxy/komodo-defi-framework that referenced this pull request Mar 30, 2024
* dev:
  feat(zcoin): balance event streaming (KomodoPlatform#2076)
  feat(zcoin): tx_history support for WASM target (KomodoPlatform#2077)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants