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

Reformat webhooks #597

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Reformat webhooks #597

merged 5 commits into from
Jul 14, 2023

Conversation

avichalp
Copy link
Collaborator

@avichalp avichalp commented Jul 13, 2023

Summary

Adds more context to webhooks content by adding hyperlinks for transactions, table ids and chain ids. Fixes GOT-108

Implementation overview

To add links to the tableland docs, and block explorers I have stored these links in the webhooks package against their corresponding chain ids.

  • Are changes backward compatible with existing SDKs, or is there a plan to manage it correctly?
  • Are changes covered by existing tests, or were new tests included?
  • Are code changes optimized for future code readers, commenting on problematic areas to understand (if any)?
  • Future-self question: Did you avoid unjustified/unnecessary complexity to achieve the goal?

@linear
Copy link

linear bot commented Jul 13, 2023

GOT-108 Add webhook handling to event processo

The hook could be defined in the chain config. Just fire at it in a go routine or similar.

Signed-off-by: avichalp <hi@avichalp.me>

Fix lint

Signed-off-by: avichalp <hi@avichalp.me>

Refactor nft views

Signed-off-by: avichalp <hi@avichalp.me>
Signed-off-by: avichalp <hi@avichalp.me>
Signed-off-by: avichalp <hi@avichalp.me>
ContractAddr common.Address
TBLDocsURL string
BlockExplorerURL string
SupportsNFTView bool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some block explorers like Etherscan have started to provide an NFT view. I believe this feature will be added to others eventually. Using this flag we can provide a link to the NFT view if available.

Signed-off-by: avichalp <hi@avichalp.me>

txnURL := chains[int64(r.ChainID)].BlockExplorerURL + "/tx/" + r.TxnHash
docsURL := chains[int64(r.ChainID)].TBLDocsURL
err = tmpl.Execute(&c, webhookContentData{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Enriches receipt with some more data such as links which will be useful for generating webhook contnet

@avichalp avichalp changed the base branch from main to staging July 14, 2023 09:39
@avichalp avichalp marked this pull request as ready for review July 14, 2023 09:39
@@ -76,14 +80,162 @@ func sendWebhookRequest(ctx context.Context, url string, body interface{}) error
return nil
}

type chain struct {
ID int64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the ID field necessary? I'm not seeing it being used and also it seems a bit redundant because the key of the map is the chain ID

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Name is also not being used

var chains = map[int64]chain{
1: {
ID: 1,
Name: "Ethereum",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you remove the Name field, you can add a comment on top of each chain to indicate the name of the chain

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, good idea. Its not being used anywhere.

func getTableNFTViews(tableIDs tables.TableIDs, chainID tableland.ChainID) string {
var tableNFTURLs []string
for _, tableID := range tableIDs {
if !chains[int64(chainID)].SupportsNFTView {
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth creating a variable for storing the value of chains[int64(chainID)], so you don't do lots map lookup later? mostly to increase readability

SupportsNFTView bool
}

var chains = map[int64]chain{
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if you change to map[tableland.ChainID]chain? does it avoid the int64 conversions?

Copy link
Collaborator

@brunocalza brunocalza left a comment

Choose a reason for hiding this comment

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

Looks good! Left some comments for consideration

Signed-off-by: avichalp <hi@avichalp.me>
@avichalp avichalp merged commit 75ff363 into staging Jul 14, 2023
5 checks passed
@avichalp avichalp deleted the avichalp/reformat-webhooks branch July 14, 2023 14:56
@avichalp avichalp mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants