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

introduce wallet activity webhook #245

Open
wants to merge 39 commits into
base: v0.7.0
Choose a base branch
from

Conversation

howard-at-cb
Copy link
Contributor

@howard-at-cb howard-at-cb commented Sep 16, 2024

What changed? Why?

  1. introduce event type filter for webhooks to support wallet activity webhook
  2. relax the test coverage a bit

tests done locally
create webhook via Webhook object

let w2 = await Webhook.create({ networkId: "base-mainnet", notificationUri: "https://e.com/callback", eventType: "wallet_activity",  eventTypeFilter: {addresses: ["0xa55C5950F7A3C42Fa5799B2Cac0e455774a07382"], wallet_id: "w1"}, eventFilters: [{ contract_address: "0x833589fcd6edb6e08f4c7c32d4f71b54bda02913" }], signatureHeader: "optional-signature-string"});
undefined
> w2.toString()
`Webhook { id: '66e9dc7bb97a771dc092d26b', networkId: 'base-mainnet', eventType: 'wallet_activity', eventFilter: [{"contract_address":"0x833589fcd6edb6e08f4c7c32d4f71b54bda02913"}], eventTypeFilter: {"addresses":["0xa55c5950f7a3c42fa5799b2cac0e455774a07382"],"wallet_id":"w1"}, notificationUri: 'https://e.com/callback', signatureHeader: 'optional-signature-string' }`

create webhook via Wallet object

  const w2 = await Wallet.create();

 let wh0 = await  w2.createWebhook('http://abc.com')
undefined
> wh0.toString()
`Webhook { id: '66eb6c01b97a771dc092d271', networkId: 'base-sepolia', eventType: 'wallet_activity', eventFilter: undefined, eventTypeFilter: {"addresses":["0x433cc6a3ef7b63d88754d7bf49efed6e8de6e813"],"wallet_id":"1690a96b-3496-4715-bc42-fa900b1ba662"}, notificationUri: 'http://abc.com', signatureHeader: '' }`

Qualified Impact

@howard-at-cb howard-at-cb changed the title Update api.ts introduce wallet activity webhook Sep 16, 2024
@howard-at-cb howard-at-cb marked this pull request as ready for review September 17, 2024 22:00
Copy link
Contributor

@John-peterson-coinbase John-peterson-coinbase left a comment

Choose a reason for hiding this comment

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

LGTM - Can you please add a CHANGELOG.md entry and then I can circle back for +1

src/coinbase/webhook.ts Outdated Show resolved Hide resolved
src/coinbase/webhook.ts Show resolved Hide resolved
@howard-at-cb
Copy link
Contributor Author

LGTM - Can you please add a CHANGELOG.md entry and then I can circle back for +1

I will do this after rebasing changes from v0.6.0. We will release this PR on v0.7.0.

src/coinbase/wallet.ts Outdated Show resolved Hide resolved
src/coinbase/wallet.ts Outdated Show resolved Hide resolved
const defaultAddress = this.model.default_address?.address_id;

// Combine the addresses into one array
const combinedAddresses = defaultAddress ? [...addressArray, defaultAddress] : addressArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't defaultAddress included in addresses already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but local testing shows this

const ws = await Wallet.listWallets()
undefined
> ws.length
3
> let w1 = ws[0]
undefined
> w1
Wallet {
  addresses: [],
  addressPathPrefix: "m/44'/60'/0'/0",
  model: {
    default_address: {
      address_id: '0x1feB1231f1e7A26036592A5E5a8E0B917B5Dc766',
      index: 0,
      network_id: 'base-sepolia',
      public_key: '03c200001c9561f31ec613b7dde214138a70e1ee47d1d75192f8e5b8584822d06d',
      wallet_id: 'dc8f00fc-f159-4d90-a255-329701c51760'
    },
    feature_set: {
      faucet: false,
      gasless_send: false,
      server_signer: false,
      stake: false,
      trade: false,
      transfer: false
    },
    id: 'dc8f00fc-f159-4d90-a255-329701c51760',
    network_id: 'base-sepolia'
  },
  master: undefined,
  seed: ''
}

Copy link
Contributor Author

@howard-at-cb howard-at-cb Sep 18, 2024

Choose a reason for hiding this comment

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

tested with

    notificationUri: string,
    signatureHeader: string = "",
  ): Promise<Webhook> {
    const addressArray = this.addresses.map(address => address.getId()!);

    return Webhook.create({
      networkId: this.getNetworkId(),
      notificationUri: notificationUri,
      eventType: WebhookEventType.WalletActivity,
      eventTypeFilter: {
        addresses: addressArray,
        wallet_id: this.getId()!,
      },
      signatureHeader: signatureHeader,
    });
  }

it seems default address was not included

wh0.toString()
`Webhook { id: '66eb348ab97a771dc092d270', networkId: 'base-sepolia', eventType: 'wallet_activity', eventFilter: undefined, eventTypeFilter: {"addresses":null,"wallet_id":"dc8f00fc-f159-4d90-a255-329701c51760"}, notificationUri: 'http://abc.com', signatureHeader: '' }`

Copy link
Contributor

Choose a reason for hiding this comment

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

default address is part of addresses in ruby sdk though

[2] pry(main)> wa = Coinbase::Wallet.list.first
=> Coinbase::Wallet{id: '7808e5ad-afff-4295-8474-fee86814a7f4', network_id: 'base_sepolia', default_address: '0x0462dbeD18478e4e7e54f32391E2cAF672Ea2144'}
[4] pry(main)> wa.addresses
=> [Coinbase::Address{id: '0x0462dbeD18478e4e7e54f32391E2cAF672Ea2144', network_id: 'base_sepolia', wallet_id: '7808e5ad-afff-4295-8474-fee86814a7f4'}]
[5] pry(main)> wh = Coinbase::Webhook.list.first
=> Coinbase::Webhook{id: '66e9be77b97a771dc092d266', network_id: 'base-sepolia', event_type: 'wallet_activity', notification_uri: 'https://david_demo.com', event_filters: '[]', signature_header: '', event_type_filter: '{:addresses=>["0x0462dbed18478e4e7e54f32391e2caf672ea2144"], :wallet_id=>"7808e5ad-afff-4295-8474-fee86814a7f4"}'}

@John-peterson-coinbase can you confirm and help us understand what's wrong here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Listing wallets with Wallet.listWallets() does not hydrate the wallet with seed if you are not using server-signer.

The addresses list will include the defaultAddress, but you must use a hydrated wallet to test.

You can either create a new wallet with Wallet.create() or you can loadSeed for the wallet that you retrieved from the list call.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 Can we consider making this so we don't rely on the SDK to pass in the full list of addresses?

Otherwise when we subsequently create addresses the SDK would need to potentially check if there are webhooks and update them.

If we do this all on the backend like we were talking about when this feature was proposed, then we can have a better source of truth and won't need to implement this logic in all of our SDKs!

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@howard-at-cb howard-at-cb Sep 18, 2024

Choose a reason for hiding this comment

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

just to make sure we are on the same page, the intention of this change is to allow users to create webhooks on the wallet level, to monitor wallet activities. When calling wallet.createWebhook() user does not need to pass in a list of addresses, the method make use of the internal list of addresses that is a instance variable of the wallet object.

When we create a new address in a wallet that already has a webhook attached, the webhook needs to be "synced". We are aware of this issue and proposed a few solutions. Initially we might just launch without this support.

Does this address your question @alex-stone ?
CC: @cb-davidlai @chaoyaji-cb

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the SDK need to pass in a list of addresses when we can just do so from the backend?

Then every time we update the wallet / create an address, we can update associated webhooks!

This is what we were discussing when talking about this feature a couple months ago!

src/tests/wallet_test.ts Outdated Show resolved Hide resolved
@alex-stone alex-stone deleted the branch v0.7.0 September 18, 2024 23:20
@alex-stone alex-stone closed this Sep 18, 2024
@alex-stone alex-stone reopened this Sep 18, 2024
@alex-stone alex-stone changed the base branch from release-v0.6.0 to v0.7.0 September 18, 2024 23:23
@alex-stone
Copy link
Contributor

Looks like this got auto-closed when we removed the old feature branch!

I targeted this against v0.7.0!

Highly recommend rebasing and squashing your commits!

@alex-stone
Copy link
Contributor

Reiterating this here:

#245 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants