Skip to content

Remove Unnecessary Event Data#18

Closed
bh2smith wants to merge 3 commits intomainfrom
17/unnecessary-event-data
Closed

Remove Unnecessary Event Data#18
bh2smith wants to merge 3 commits intomainfrom
17/unnecessary-event-data

Conversation

@bh2smith
Copy link
Copy Markdown
Member

Closes #17

bytes32 indexed id,
address indexed subscriber,
address indexed recipient,
uint256 lastRedeemed,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we actually want to have nextRedeemAt here. Otherwise the indexer will have to compute next redeem.

See here:

https://github.com/deluXtreme/subindexer/blob/0fa24bbd583ed5bc4f9e39ff79ad7e5e99077582/src/redeemable-subscriptions.sql#L15-L22

Copy link
Copy Markdown
Contributor

@lumoswiz lumoswiz Jun 14, 2025

Choose a reason for hiding this comment

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

True, except the current sub.lastRedeemed is equivalent to nextRedeemAt. It gets updated in _applyRedemption. It's also important that, when there are multiple periods being potentially redeemed, it is incremented by (number periods x frequency) & not via block.timestamp.

Correct me if I'm wrong though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think renaming the current lastRedeemed param to nextRedeemAt makes sense, but keep how it's currently being used/set.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I saw that last redeemed was timestamp minus frequency. But next redeem at is timestamp plus frequency. I guess it's also ok to leave this, and the sql query could be adjusted to add the two fields together.

@lumoswiz
Copy link
Copy Markdown
Contributor

lumoswiz commented Jul 3, 2025

Closing. Added in group subscription overhaul, see #16.

@lumoswiz lumoswiz closed this Jul 3, 2025
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.

Remove Unnecessary Event Field

2 participants