Skip to content

Conversation

dmanc
Copy link
Contributor

@dmanc dmanc commented Jul 2, 2025

Why are these changes needed?

Adds a subgraph for the PaymentVault. Mainly used to get observability into reservation expirations.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@dmanc dmanc changed the title Payments subgraph feat: Payments subgraph Jul 2, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you copy-paste this in here or is this auto-generated? Should prob be auto-generated from our contracts and have a ci to check that its still up-to-date?

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 compiled the contracts and copy pasted it here, see README.md at the root of the directory. Think it's a good idea for checking if the abi is up to date and failing some check if it isn't, will add it if it's straightforward.

Copy link
Contributor Author

@dmanc dmanc Jul 7, 2025

Choose a reason for hiding this comment

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

Added the checks here 2fe68a0. Seems like the ABIs have indeed changed but shouldn't affect the existing subgraphs at all.

I do think the check is useful so that anytime any functionality is added to the contract we can check and see if anything useful can be added to the subgraph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be using IPaymentVault instead of PaymentVault. Changes less often, and we only need the interface anyways. I think I'm fine with this approach (although it does look like a lot of hard to maintain ai generated code.. so I hope it won't be flakey in CI), but @cody-littley has always been against checking in compiled artifacts into the repo, so wondering if this complexity could all have been alleviated by just having a makefile command to go compile the contracts and use the generated artifacts from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

talked online and we decided to remove the ABIs since they're really only used for bootstraping the subgraph 451a0bf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops it actually does need it when running the code generation

transactionHash: Bytes!
}

type CurrentReservation @entity(immutable: false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you mentioned on slack that this is the only entity that you added by hand? Maybe add a big ============= HAND-WRITTEN BELOW ========= demarcation comment to know what is auto-generated and what is hand-written?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mentioned in the other comment not wanting to distinguish what is auto-generated vs manually generated. I think I agree, though I still would like to be able to easily distinguish which parts of the schema are purely mapping 1:1 to onchain events, and which ones are derived state (aka only this CurrentReservation). So I think I would still want something like

# Everything above here maps 1:1 to onchain events
================== EVENT-DERIVED STATE BELOW ==============

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sg, will add that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 154 to 156
let activeReservation = CurrentReservation.load(event.params.account)
if (activeReservation == null) {
activeReservation = new CurrentReservation(event.params.account)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do these currentReservations ever get deleted? How does the subgraph know that its still current? DO we have to filter manually for endTimestamp?

Copy link
Contributor

Choose a reason for hiding this comment

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

reservations never get deleted on-chain, so we need to filter. I would recommend adding a status enum type for active, expired or inactive since there could be reservation that gets activated in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will use the following logic

enum ReservationStatus {
  PENDING    # startTimestamp > current time
  ACTIVE     # startTimestamp <= current time < endTimestamp
  EXPIRED    # endTimestamp <= current time
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually after trying to add this status enum I realized it doesn't really make sense to do on the subgraph side since right now we only update the CurrentReservation entity every time there's a ReservationUpdated event. One potential solution is to add a block handler that loops over all existing reservations but I feel that it's starting to make the subgraph more complicated.

Currently, the semantics of CurrentReservation is to give the most up to date reservation for a given account. I would prefer to leave the filtering (based on the logic in the previous comment) to the clients of the subgraph.

e.g.

# Active reservations
query ActiveReservations($now: BigInt!) {
  currentReservations(
    where: {
      startTimestamp_lte: $now,
      endTimestamp_gt: $now
    }
  ) {
    account
    symbolsPerSecond
  }
}

# Pending reservations
query PendingReservations($now: BigInt!) {
  currentReservations(
    where: { startTimestamp_gt: $now }
  ) {
    account
    startTimestamp
  }
}

# Expired reservations
query ExpiredReservations($now: BigInt!) {
  currentReservations(
    where: { endTimestamp_lte: $now }
  ) {
    account
    endTimestamp
    symbolsPerSecond
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, that's okay with me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep that makes a ton of sense, nice md! Can we just change currentReservations to simply reservations everywhere then, since expired reservations are not deleted, so they aren't "current"? Or maybe registeredReservations or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with Reservation 6de3851

@dmanc dmanc marked this pull request as ready for review July 7, 2025 22:05
@dmanc dmanc requested review from samlaf and hopeyen July 7, 2025 22:05
@dmanc dmanc merged commit d480925 into master Jul 8, 2025
18 of 19 checks passed
@dmanc dmanc deleted the payments-subgraph branch July 8, 2025 20:09
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.

3 participants