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

feat: TAP Agent #83

Merged
merged 18 commits into from
Nov 14, 2023
Merged

feat: TAP Agent #83

merged 18 commits into from
Nov 14, 2023

Conversation

aasseman
Copy link
Contributor

@aasseman aasseman commented Oct 26, 2023

Sorry, it's a big one. Perhaps we could set up a call to discuss this PR.

The TAP Agent is a new daemon that indexers will have to run alongside the indexer-service instances. It:

  • Monitors receipts being saved by the indexer-service instances into the DB (through pg_notify).
  • Keeps track of the unaggregated fees for each Account, where an Account is (indexer, sender, allocation).
  • Handles the receipt validation checks (because the indexer-service is designed to do barely any checks).
  • Handles RAV (Receipt Aggregate Vouchers) requests whenever the total unaggregated fees surpass a trigger amount.
  • Automatically does one last RAV request when an Account relationship ends (Allocation closed, for example).

What it should, but does not do yet:

  • Check the receipt values against the Agora system
  • Signal the indexer-service that a sender should be banned (not enough in Escrow, too many invalid receipts, etc)
  • A lot of other, smaller, but important things, that are marked as TODO in the code...

I'm not particularly proud of the overall design, and it also shone some light onto design shortcomings in tap_core that we did not foresee while designing it "in a vacuum". That's why there is some weirdness in the TAP adapters implementation such as https://github.com/graphops/indexer-service-rs/blob/20e48b1191a8d39f02e32200ef75fc065470cdd6/tap_agent/src/tap/receipt_checks_adapter.rs#L64-L76.

My goal here is to get something through the door so that we can continue improving this incrementally together. I propose that we try moving as many review issues into actual issues that we tackle later.

@aasseman aasseman added size:x-large Very large p1 High priority type:feature New or enhanced functionality labels Oct 26, 2023
@aasseman aasseman requested review from Jannis and hopeyen October 26, 2023 21:07
@aasseman aasseman self-assigned this Oct 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2023

Pull Request Test Coverage Report for Build 6698506386

  • 867 of 1235 (70.2%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+8.4%) to 58.656%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tap_agent/src/tap/test_utils.rs 66 68 97.06%
tap_agent/src/tap/rav_storage_adapter.rs 39 45 86.67%
tap_agent/src/main.rs 0 15 0.0%
tap_agent/src/tap/escrow_adapter.rs 87 102 85.29%
tap_agent/src/database.rs 0 22 0.0%
tap_agent/src/tap/receipt_storage_adapter.rs 182 211 86.26%
tap_agent/src/tap/receipt_checks_adapter.rs 107 149 71.81%
tap_agent/src/tap/account.rs 222 265 83.77%
tap_agent/src/tap/accounts_manager.rs 127 183 69.4%
tap_agent/src/config.rs 8 69 11.59%
Totals Coverage Status
Change from base Build 6644835579: 8.4%
Covered Lines: 2348
Relevant Lines: 4003

💛 - Coveralls

common/src/tap_manager.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

Great work! still need to digest the tap details, would you mind sharing the flow diagram somewhere? 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

should there be a ...tap_receipts.down.sql migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's there, I just didn't change the version from main: https://github.com/graphops/indexer-service-rs/blob/407c17d3ecf766c8e37a2b7e4b9b1132136c4a33/migrations/20230912220523_tap_receipts.down.sql

Also, I modified the migrations in place instead of creating new ones. I thought at this stage of development we don't need to care about migrating the database. (tell me if that's the wrong thinking though)

SubgraphClient::new(
config
.network_subgraph
.network_subgraph_deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems complicated for making a subgraph client. Maybe we can later on make an easier creation helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the new API introduced in #81

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it looks complicated. Perhaps the DeploymentDetails I added into the API is making it a little verbose. Might be worth cleaning up a little at some point, but doesn't feel urgent.

)]
pub rav_request_timestamp_buffer_ns: u64,

// TODO: Remove this whenever the the gateway registry is ready
Copy link
Collaborator

Choose a reason for hiding this comment

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

how do we expect indexers to have this file? does senders register or host a discoverable aggregator endpoint somewhere? would you please provide an example file I could run the service with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be in the form of a smart contract. That's the current plan/suggestion I got from @Theodus . But no work started on this yet I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: the file is just to be able to get going for testing, basically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, ok, if you got this from @Theodus, then that answers my earlier question about which contract we're talking about - there's just one. 😄

I think it's fine to keep this file as a temporary approach.

as Receipt Aggregate Voucher (RAV) requests for an indexer."
)]
#[command(author, version, about, long_about = None, arg_required_else_help = true)]
pub struct Cli {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of these are duplicates of indexer-service CLI, would it make sense to refactor the duplicates to indexer-common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Not sure there is an elegant way to do that though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest leaving these duplicates in for the moment. One thing I'm doing with the indexer-service framework is to have a common config file structure (TOML) and allow indexer service implementations to add their own structure to that (within the same TOML file).

But indexer-service and indexer-agent and tap-agent are all their own processes and I think it's better to keep their interfaces independent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, from now on I'll advocate using as few CLI arguments as possible and moving everything to a config file. It is more secure (env vars are really bad for passing in private keys, mnemonics, passwords) and also easier to manage in systems like terraform, kubernetes etc.

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 in k8s, the preferred way of passing secrets is using env vars. Unless you are willing to integrate your software directly with the k8s or your cloud's secrets API.
My favorite config implementation is that of graph-node. It uses TOML, but also does something like envsubst over the file before loading it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.cloudtruth.com/blog/the-pitfalls-of-using-environment-variables-for-config-and-secrets suggests that environment variables for secrets is insecure.

I'm not sure this really applies to containers though. This is still one of the recommended ways in k8s for ex: https://kubernetes.io/docs/tasks/inject-data-application/distribute-credentials-secure/

@aasseman
Copy link
Contributor Author

Great work! still need to digest the tap details, would you mind sharing the flow diagram somewhere? 🙏

Working on it!

@aasseman
Copy link
Contributor Author

aasseman commented Nov 1, 2023

Sorry for the delay. Here's a simple diagram. Which part would you like more details for?

tap_agent

The green blocks are Eventuals

Copy link
Collaborator

@Jannis Jannis left a comment

Choose a reason for hiding this comment

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

Made it 30% through. Will keep going but wanted to drop my comments so far. 😉

migrations/20230915230734_tap_ravs.up.sql Outdated Show resolved Hide resolved
tap_agent/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 69 to 72
// TODO: replace with a proper implementation once the gateway registry contract is ready
let sender_aggregator_endpoints = aggregator_endpoints::load_aggregator_endpoints(
config.tap.sender_aggregator_endpoints_file.clone(),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once gateways register somewhere (they will do this for subscriptions — perhaps that's the contract you're referring to here or is there another one for TAP?), this could make for a nice Eventual in the indexer service. Add the TAP agent gRPC API, pipe the gateways eventual into an eventual that polls the "need a RAV?" status for each gateway periodically and whenever the gateways change, pipe that into code that requests the RAVs and stores them back in the DB. Done? 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saved your comment in the appropriate issue: #84 (comment)

as Receipt Aggregate Voucher (RAV) requests for an indexer."
)]
#[command(author, version, about, long_about = None, arg_required_else_help = true)]
pub struct Cli {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest leaving these duplicates in for the moment. One thing I'm doing with the indexer-service framework is to have a common config file structure (TOML) and allow indexer service implementations to add their own structure to that (within the same TOML file).

But indexer-service and indexer-agent and tap-agent are all their own processes and I think it's better to keep their interfaces independent.

as Receipt Aggregate Voucher (RAV) requests for an indexer."
)]
#[command(author, version, about, long_about = None, arg_required_else_help = true)]
pub struct Cli {
Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, from now on I'll advocate using as few CLI arguments as possible and moving everything to a config file. It is more secure (env vars are really bad for passing in private keys, mnemonics, passwords) and also easier to manage in systems like terraform, kubernetes etc.

Comment on lines 190 to 196
pub async fn start_last_rav_request(&self) {
*(self.inner.state.lock().await) = State::LastRavPending;
let mut rav_requester_task = self.rav_requester_task.lock().await;
if !Self::rav_requester_task_is_running(&rav_requester_task) {
*rav_requester_task = Some(tokio::spawn(Self::rav_requester(self.inner.clone())));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a reason to rework this PR but it would be nice to avoid the state machine and all the locking happening in Account. But I'd need to think how for a bit, not sure right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's complex enough that we could have a risk of an unforeseen deadlock.

That was the best I could come up with to make sure that:

  • Only one RAV request happens at a time
  • The RAV request is async from all the rest
  • I can set the "last rav" state asynchronously
  • The RAV request can clear the state on its own (while living in its own async bubble)

Sorry, I guess I default back to Electrical Engineer mode in situations like this 😁

Copy link
Collaborator

Choose a reason for hiding this comment

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

An mpsc::channel with a size of 1 could perhaps work to ensure there's only one RAV request processed and it's processed somewhere else. Buuuuuut, I'm not sure. I don't think it works, as reading the next RAV request from the receiver is enough to allow the next one in. And you want more coordination to happen there. I'll think about it some more, but this can be improved later if needed.

AND sender_address = $2
)
SELECT
COUNT(*),
Copy link
Collaborator

Choose a reason for hiding this comment

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

How many receipts do we think this would at max count over? count(*) and the other aggregations too are pretty slow; I'd like to get an idea of how slow this query could get.

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 estimate that for a regular indexer, should be rarely above ~1000s, as there should be only the unaggregated receipts in there.

Also, I believe things could be optimized such that this query would happen only once at TAP Agent start.

Comment on lines 254 to 255
// `COUNT(*)` will always return a value, so we don't need to check for `None`.
match res.count.unwrap() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think something like the below might be preferable to querying count(*) only to avoid the check for None:

unaggregated_fees.last_id = res.max.unwrap_or(0).try_into()?;
unaggregated_fees.value = res.sum.unwrap_or(0).to_string().parse::<u128>()?;

Copy link
Contributor Author

@aasseman aasseman Nov 2, 2023

Choose a reason for hiding this comment

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

The actual reason I wanted the count was to run those checks:

ensure!(
res.max.is_some(),
"MAX(id) is null but the receipt COUNT(*) is not zero"
);
ensure!(
res.sum.is_some(),
"SUM(value) is null, but the receipt COUNT(*) is not zero"
);

That's indeed a bit overzealous, and a check like this should be more than enough:

ensure!(
    res.sum.is_none() == res.max.is_none(),
    "Exactly one of SUM(value) and MAX(id) is null. This should not happen."
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8515c03

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! (And good point about the checks, I missed that in my oversimplified suggestion.)

}

async fn rav_requester_try(inner: &Arc<Inner>) -> anyhow::Result<()> {
loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to this: do we make sure that we don't request multiple RAVs for the same receipts in parallel if a new receipt comes in before the RAV comes back and we still have unaggregated fees > threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • We do make sure that we don't do RAV requests in parallel for any given (sender,allocation), by checking Self::rav_requester_task_is_running before spawning a RAV request task. Could be made more elegant I think.

  • Each RAV request must contain the last RAV (unless it is the first RAV request). We basically aggregate the receipts into the latest RAV we got, and we get a new RAV out of that. That's why it is very important the the whole RAV request dance is serial.

  • The TapManager will only aggregate receipts that have a timestamp more recent than the last RAV (though note that we are OK with multiple receipts having the same timestamp, they also have a nonce). To make sure that there is no confusion with the new receipts coming in while we're requesting a RAV, the TapManager will only aggregate receipts up to timestamps that are more than timestamp_buffer old: https://github.com/semiotic-ai/timeline-aggregation-protocol/blob/656fc15700f6d29299d78f9c790539cbddb68d02/tap_core/src/tap_manager/manager.rs#L179

    One issue I can see here is that we are not checking (in indexer-service) that new receipts have a reasonably accurate timestamp (let's say within 5 seconds of our own clock). So as it is right now it is not really safe :/.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be useful to track locking this in better via an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, created and issue: #92

Comment on lines 306 to 307
// TODO: Request compression and response decompression. Also a fancy user agent?
let client = HttpClientBuilder::default().build(&inner.sender_aggregator_endpoint)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious: Say you receive 5000 receipts/s from a gateway, each for a value of $0.0001 and your threshold is $100. 5000 receipts/s make for $0.5/s. So you'd hit the threshold after 200s, at which point you'll have 1M receipts to aggregate.

My questions would be:

  • How long will the SQL query to calculate the sum of the 1M unaggregated receipts take roughly?
  • How many MBs of payload is this to send to the gateway?
  • How long will it take the gateway to aggregate 1M receipts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We set some standards here: https://github.com/semiotic-ai/timeline-aggregation-protocol/blob/main/tap_aggregator/README.md

We recommend a max of 15,000 receipts / RAV request.

So here are the missing bits currently:

  • In tap_core's Manager, we should be able to specify a maximim number of receipts / RAV request
  • In TAP agent, we should trigger RAV requests not only a value, but also a number of receipts (< 15,000)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like it would be good to have an issue for this. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's say we had 500 indexers all receiving 5,000 receipts/s from a single gateway (the numbers are higher than we'd see them right now, but one day...), this gateway would have 167 RAVs to generate for 15,000 receipts each, per second. Would it be able to keep up? 🤔

Copy link
Contributor Author

@aasseman aasseman Nov 14, 2023

Choose a reason for hiding this comment

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

Created issue #96

Signed-off-by: Alexis Asseman <[email protected]>
Was `scalar_tap_latest_ravs` everywhere except in the down migration.
Opted for the shorter of the 2 everywhere.

Signed-off-by: Alexis Asseman <[email protected]>
Instead of underscores.

Signed-off-by: Alexis Asseman <[email protected]>
Signed-off-by: Alexis Asseman <[email protected]>
In particular, simplify handling of NULL results from the DB.

Signed-off-by: Alexis Asseman <[email protected]>
Signed-off-by: Alexis Asseman <[email protected]>
Signed-off-by: Alexis Asseman <[email protected]>
@aasseman
Copy link
Contributor Author

Rebased onto latest main (at least so that the tests don't fail)

@aasseman aasseman requested a review from Theodus November 11, 2023 00:18
Copy link
Contributor

github-actions bot commented Nov 11, 2023

Pull Request Test Coverage Report for Build 6857909753

  • 913 of 1288 (70.89%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+8.6%) to 59.005%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tap-agent/src/tap/test_utils.rs 66 68 97.06%
tap-agent/src/tap/rav_storage_adapter.rs 39 45 86.67%
tap-agent/src/main.rs 0 15 0.0%
tap-agent/src/tap/escrow_adapter.rs 87 102 85.29%
tap-agent/src/database.rs 0 22 0.0%
tap-agent/src/tap/receipt_storage_adapter.rs 182 211 86.26%
tap-agent/src/tap/sender_allocation_relationship.rs 221 262 84.35%
tap-agent/src/tap/receipt_checks_adapter.rs 107 149 71.81%
tap-agent/src/tap/sender_allocation_relationships_manager.rs 134 197 68.02%
tap-agent/src/config.rs 48 113 42.48%
Totals Coverage Status
Change from base Build 6820857884: 8.6%
Covered Lines: 2408
Relevant Lines: 4081

💛 - Coveralls

Copy link
Collaborator

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

The overall structure makes sense to me. Would you be able to provide a list (maybe in docs or something) that tracks todo/unimplemented items so we can have a better view of the progress? I think we should also provide a link to the docs describing the lifetime of a receipt/rav

tap-agent/src/agent.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/receipt_checks_adapter.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/receipt_checks_adapter.rs Outdated Show resolved Hide resolved
type AdapterError = AdapterError;

/// We don't need this method in TAP Agent.
async fn store_receipt(&self, _receipt: ReceivedReceipt) -> Result<u64, Self::AdapterError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What component calls this function and update_receipt_by_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It the TapManager from tap_core: https://github.com/semiotic-ai/timeline-aggregation-protocol/blob/3d247c06380038d0a871a7427ce00e0406909160/tap_core/src/tap_manager/manager.rs#L80-L111.

But we don't need those since the storage of receipts is done only in service, and in a mush simplified way, so it doesn't even leverage the TapManager at all.

let updated_rows = sqlx::query!(
r#"
UPDATE scalar_tap_latest_ravs
SET is_last = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should call this "closing RAV" or the "final RAV" or simply "is_last_for_allocation_and_sender" to be very explicit. Something like that. "last" can have so many meanings: the last received, the last in some other ordering (I first wanted to comment that we can find the last easily and quickly via the timestamp, but then I realized last has a different meaning here), the last for the allocation/sender...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1178c63

SubgraphClient::new(
config
.network_subgraph
.network_subgraph_deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it looks complicated. Perhaps the DeploymentDetails I added into the API is making it a little verbose. Might be worth cleaning up a little at some point, but doesn't feel urgent.

as Receipt Aggregate Voucher (RAV) requests for an indexer."
)]
#[command(author, version, about, long_about = None, arg_required_else_help = true)]
pub struct Cli {
Copy link
Collaborator

Choose a reason for hiding this comment

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

tap_agent/src/main.rs Outdated Show resolved Hide resolved
unaggregated_fees.last_id = new_receipt_notification.id;

let mut rav_requester_task = self.rav_requester_task.lock().await;
// TODO: consider making the trigger per sender, instead of per (sender, allocation).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Potentially. Now that you talk about it like it's a scheduling algorithm, I'm thinking we should perhaps merge the logic as is and play with different "algorithms" using realistic amounts of queries, receipts and thresholds. But that's more about optimizing RAV requests in the face of millons of receipts than it is about how an indexer would define a threshold. Hm...

Comment on lines 306 to 307
// TODO: Request compression and response decompression. Also a fancy user agent?
let client = HttpClientBuilder::default().build(&inner.sender_aggregator_endpoint)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's say we had 500 indexers all receiving 5,000 receipts/s from a single gateway (the numbers are higher than we'd see them right now, but one day...), this gateway would have 167 RAVs to generate for 15,000 receipts each, per second. Would it be able to keep up? 🤔

tap-agent/src/config.rs Outdated Show resolved Hide resolved
tap-agent/src/config.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/receipt_checks_adapter.rs Show resolved Hide resolved
tap-agent/src/tap/escrow_adapter.rs Outdated Show resolved Hide resolved
@aasseman
Copy link
Contributor Author

The overall structure makes sense to me. Would you be able to provide a list (maybe in docs or something) that tracks todo/unimplemented items so we can have a better view of the progress? I think we should also provide a link to the docs describing the lifetime of a receipt/rav

For the progress, I created a milestone here: https://github.com/graphprotocol/indexer-rs/milestone/1. I'm going to add all the todos there.

Copy link
Collaborator

@Jannis Jannis left a comment

Choose a reason for hiding this comment

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

This is good to go from my point of view. Thanks for addressing the feedback so quickly!

@aasseman aasseman requested a review from hopeyen November 14, 2023 20:02
@aasseman
Copy link
Contributor Author

This is good to go from my point of view. Thanks for addressing the feedback so quickly!

Thanks for taking the time to go through the big PR 🙏 !

@aasseman aasseman merged commit d2fad80 into main Nov 14, 2023
5 checks passed
@aasseman aasseman deleted the aasseman/tap-agent branch November 14, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 High priority size:x-large Very large type:feature New or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants