allow updating address list of a wallet activity webhook #280
Conversation
jazz-cb
requested review from
rohan-agarwal-coinbase and
John-peterson-coinbase
| public async update({ | ||
| notificationUri, | ||
| eventTypeFilter, | ||
| }: UpdateWebhookOptions): Promise<Webhook> { |
There was a problem hiding this comment.
Just a note that we're breaking back compat by changing the params here. Not sure if this would be an issue at this point, though.
There was a problem hiding this comment.
this is a good call out cc @jazz-cb not sure if it matters at this point
or maybe we can make the param like this: notificationUriOrOptions: string | UpdateWebhookOptions
then we can check their input: typeof notificationUriOrOptions === 'string'
There was a problem hiding this comment.
Let's note it in the release logs.
For update functions lets make sure to use Objects for this forward-compatibility!
CHANGELOG.md
Outdated
| ## [0.8.0] - 2024-10-03 | ||
|
|
||
| ### Breaking | ||
| - `Webhook#update` now takes an Object of `{notificationUri, eventTypeFilter}` instead of a string of notification Uri. |
There was a problem hiding this comment.
Webhook#update now takes an Object of {notificationUri, eventTypeFilter} instead of notificationUri string
| @@ -2,6 +2,11 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ## [0.8.0] - 2024-10-03 | |||
|
|
|||
| ### Breaking | |||
There was a problem hiding this comment.
could you also add the instructions to docs
wh1.update({eventTypeFilter: {addresses:["0x40A28c0fCc0BE09400bB89CdF556Cd8C4eF1c165","0x6f03b3Df22F0C57A4477EEAc3a49c2Bc4EAe2206"]}})
There was a problem hiding this comment.
There was a problem hiding this comment.
if that is the place to put documentation then this change is documented. cc: @jazz-cb
| * @param notificationUri - The new URI for webhook notifications. | ||
| * @param options - The options to update webhook. | ||
| * @param options.notificationUri - The new URI for webhook notifications. | ||
| * @param options.eventTypeFilter - The new eventTypeFilter that contains a new list (replacement) of addresses to monitor for the webhook. |
There was a problem hiding this comment.
if eventTypeFilter can only contain replacement addresses, should this be called just addresses?
Also replacement is confusing. Aren't you adding new addresses to the webhook and not replacing the old ones?
There was a problem hiding this comment.
- I thought about the naming
eventTypeFilter, my thought process is that in future we might add more stuff intoeventTypeFilter, and we might want to allow clients to update them, so keeping this name for the purpose of extensibility. - It is actually a
PUTnot aPATCH, so address array passed in will replace the existing addresses in the webhook. I clarified it in this doc PR https://github.cbhq.net/solutions-architecture/docs/pull/715/files#diff-e948e3fa52983fe8dee3cacd990a72989a39f539973f07fca0f2065dc7a0577dR15
This might not be the ideal solution, we can follow up with changes to this behavior.
* test pass * optional notif URI * Update wallet.ts * Update CHANGELOG.md * Update CHANGELOG.md
* update rewards api to latest spec * update generated client * update SDK's listAddressTransaction (#260) * listAddressTransaction * Update coinbase.ts * add examples * allow updating address list of a wallet activity webhook (#280) * test pass * optional notif URI * Update wallet.ts * Update CHANGELOG.md * Update CHANGELOG.md * feat(PSDK-535): Explicit Pagination for list Methods (#282) * feat(PSDK-535): Explicit Pagination for list Methods * address comments * chore: Release v0.8.0 (#283) --------- Co-authored-by: Filipe Azevedo <[email protected]> Co-authored-by: cb-howardatcb <[email protected]> Co-authored-by: Filipe Azevedo <[email protected]>
What changed? Why?
Qualified Impact