Webhook notifications#884
Conversation
asutula
left a comment
asutula
left a comment
There was a problem hiding this comment.
Nice work so far. The way you've written the code is easy to follow and understand, so I don't have many comments on the code for now (this is a good thing).
I see you injected the Notifier into the deals module. That is a fine place to start, but I'm guessing there will be a different place we may want to leverage the Notifier, or at least an additional place or two.
If I'm remembering things correctly, the Scheduler will be a very good place to use Notifier. The Scheduler manages all things related to storing, retrieving, renewing and repairing deals. For this reason, it will be a source for many notifications. I'd imagine those events might be higher level with less detailed information, so maybe we augment those notifications with finer grained notifications from the deals module or any other module that makes sense.
So, sounds like we should dig into the Scheduler and understand how it works and how notifications fit in.
|
|
||
| type WebhookAlert struct { | ||
| Type string | ||
| Threshold string |
There was a problem hiding this comment.
Curious to see what you're thinking for Threshold use cases. Sounds interesting.
asutula
left a comment
asutula
left a comment
There was a problem hiding this comment.
Coming along really nicely. It seems like everywhere you're integrating the notifier into the system makes sense to me. I like the implementation of the notifier and the way you define notification types, process events, and calculate notification to send. Nice work.
| Miners []string | ||
| WalletAddress string | ||
| MaxPrice uint64 | ||
| Notifications []*ffs.NotificationConfig |
There was a problem hiding this comment.
I'm curious why this addition is only in RetreivalAction and not StorageAction, but maybe I'll learn this later in my review.
There was a problem hiding this comment.
StorageAction already contains ffs.StorageConfig, which has Notifications []*NotificationConfig
| type configStore struct { | ||
| sync.RWMutex | ||
|
|
||
| configs map[ffs.JobID][]*ffs.NotificationConfig | ||
| } |
There was a problem hiding this comment.
Will this need to write to a persistent store (database) at some point?
There was a problem hiding this comment.
yes, I think at some point would be better to put it to persistent store, for the beginning I guess we can keep it in memory
| - POWD_DEVNET=true | ||
| - POWD_LOTUSHOST=/dns4/lotus/tcp/7777 | ||
| - POWD_IPFSAPIADDR=/dns4/ipfs/tcp/5001 | ||
| - POW_GATEWAYHOSTADDR=0.0.0.0:7001 |
There was a problem hiding this comment.
Just curious why these changes popped up (and in docker-compose.yaml as well). Maybe something you needed for local testing? If it's generally useful, feel free to keep the changes.
There was a problem hiding this comment.
yes, it's just for local testing, would need to revert it back
when I tested locally found that on my laptop some battery management uses this port :)
2 participants
Draft