Enable wallet activity events support for webhooks at wallet level #189
Conversation
✅ Heimdall Review Status
|
chaoyaji-cb
changed the title
lib/coinbase/wallet.rb
Outdated
| # defaulting to an empty string. | ||
| # | ||
| # @return [Coinbase::Webhook] The newly created webhook instance. | ||
| def create_webhook(notification_uri:, addresses: self.addresses.map(&:id), signature_header: '') |
There was a problem hiding this comment.
are we providing some sort of override to wallet addresses?
i feel they should not be able to do that... if custom group of addresses needs to be provided they can just go through webhook object
There was a problem hiding this comment.
Should we disallow clients to specify addresses when calling wallet.create_webhook() ? We can use the wallet's existing addresses.
| notification_uri: notification_uri, | ||
| event_type: Coinbase::Webhook::WALLET_ACTIVITY_EVENT, | ||
| event_type_filter: { | ||
| addresses: addresses.map(&:id), |
There was a problem hiding this comment.
Can the addresses be determined by the backend?
Otherwise when we create a new address, the user would need to update the webhook or the backend would need to manage it anyways
| @@ -21,10 +21,12 @@ | |||
| let(:second_address_model) do | |||
| build(:address_model, network_id, :with_seed, seed: seed, wallet_id: wallet_id, index: 1) | |||
| end | |||
| let(:webhook_model) { build(:webhook_model, :wallet_activity) } | |||
There was a problem hiding this comment.
If this is only used in the #create_webhook test, please define it in that test.
The let should generally be defined in the most narrow scope possible (e.g. the top-level lets are referenced by multiple test cases or in a top-level before block_
What changed? Why?
wallet_activityevent type withevent_type_filter->wallet_activity_filterthat tracks wallet addresses with wallet idwallet.create_webhook()Qualified Impact