-
Notifications
You must be signed in to change notification settings - Fork 505
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
Feature Request: Expose the transaction and operation(s) that created the claimable balance #3305
Comments
I think a CB should also link directly to the transaction that created it too so that clients that wish to read the memo out of the creating transaction can do so in a single hop without having to go via the operation to the transaction. This concern was raised on the stellar-dev mailing list here. |
I would favor simply including the transaction (and maybe the operation index) and operation id in the the |
For instance: https://horizon.stellar.org/claimable_balances/00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9 would now be something in the lines of: {
"_links": {
"self": {
"href": "https://horizon.stellar.org/claimable_balances/00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9"
}
},
"id": "00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9",
"asset": "BODHI:GDCJIHD3623OCYNH65UUQC3NLG2D6YCNCDPZULRLCLOA76TBQRL6A3TF",
"amount": "0.1000000",
"sponsor": "GDCJIHD3623OCYNH65UUQC3NLG2D6YCNCDPZULRLCLOA76TBQRL6A3TF",
"last_modified_ledger": 32747318,
"last_modified_time": "2020-11-23T16:02:38Z",
"claimants": [
{
"destination": "GBEUDKANIFPTFHPWJ5T3R6RIO36RQBFGHYPAQ6STH7KMNDHAT36LHOLD",
"predicate": {
"unconditional": true
}
},
],
"origin": {
"transaction_id": "thisshouldbeatransactionhash",
"operation_id": "thisshouldbeanoperationid"
},
"paging_token": "32747318-00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9"
} where the delta from the current response is: --- old.json 2021-03-09 16:51:13.000000000 +0100
+++ new.json 2021-03-09 16:51:06.000000000 +0100
@@ -16,7 +16,11 @@
"predicate": {
"unconditional": true
}
- }
+ },
],
+ "origin": {
+ "transaction_id": "thisshouldbeatransactionhash",
+ "operation_id": "thisshouldbeanoperationid"
+ },
"paging_token": "32747318-00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9"
} |
Could we also include both in the _links so that it is possible to call functions in the SDKs to get the creating and claiming transactions and creating and claiming operations? |
Placing them in _links is more consistent with how other endpoints refer to relationships too, and so you probably dont need them in the response body as well. |
good point, you mean something like the following?
In that case I wouldn't include it in the body at all. |
This could be challenging though:
If we want to persist that context, even after the claimable balance is claimed, we may have a problem (I didn't check what's Horizon's behavior on claimed claimable balances but I think we 404). For one, I am not really in favor of persisting Claimable Balances (unlike accounts, they are meant to be ephemeral). |
Your example of incorporating then into _links looks good. Good point about the 404. I agree the claimable balance should continue to 404 in the same way that accounts do after they have been deleted/merged. We need |
@leighmcculloch, to be clear, you're suggesting that after a claimable balance is gone the results should be this?
That seems like it would be surprising for unfamiliar devs. I'd suggest we either 404 everything nested under a claimable balance, or keep the claimable balance and mark it's new state somehow. Edit: What data do you need from |
@paulbellamy I agree, it is not overly intuitive, but it is consistent with how the |
An application needs to be able to review effects, operations, or transactions, for itself, and traverse backwards to understand that transaction came to be. For example, if my account claimed a claimable balance I should be able to find the transaction that created it, the account that created it, the operation that created it, etc. This is particularly important for two-part payments because the memo in the original transaction will have meaning because they get used for virtual accounts. Also, just because a claimable balance is gone it should still be possible to get at its history, otherwise knowing a claimable balance ID a client cannot find the operations, effects, etc that it interacted with after it is claimed. The historical component is important because Horizon is the primary way history is accessed. It's not so much about preserving the data, because the transactions, operations, and effects are already preserved by Horizon, it is about preserving the relationships so that they can still be looked up in relation to each other and in relation to a claimable balance that has been claimed. One other example is where claimable balances have more than one claimant a claimant may need to lookup who claimed a claimable balance if another claimant claimed. The sub-endpoints are consistent with accounts and other endpoints. Why wouldn't we want to continue to apply the same pattern here? |
I think that is completely overkill only for claimed claimable balances. I need real, specific usecases of it needed today to justify it. Most of the usecases you present are merely theoretical. |
Because it complicates the API. Particularly since, as you already mentioned, they are other (agreed, more complicated, but can be hidden by the SDK) ways to gather a claimed claimable balance. Adding three endpoints just to gather information about a claimed claimable balances seems overkill. Another possibility (without checking how complicated that would be in practice), is adding a flag to the |
I'm definitely +1 for consistency. If claimable balances are a thing we need to refer to and look up after they are claimed/deleted, then it seems to me that they're not actually ephemeral in practice. So we should persist them (not 404 them), and tombstone them in some way. |
I don't think we should limit the API we create to narrow use cases, especially when we have existing API patterns like the
I think get the creator case is addressed well by #3380 for claimable balances that still exist. @tyvdh Has shared feedback in Slack about the gaps in the existing endpoints and can probably also share more use cases. I think his feedback related to how it is difficult to query for claimable balances that you created, which would be addressed by #3306. For the two use cases, knowing how the accounts endpoints work, I would assume I could do something like:
Regarding inventing a new way to get deleted objects in the API with If we went that route we'd probably need to include a new state field on the response as well so when getting the include_claimed version you could see which were claimed or not. For the two use cases, I would assume it would work something like this:
It seems to work, but doesn't build on existing patterns of how accounts work, and it's not clear to me how it will evolve to other use cases we don't know about. |
@paulbellamy I think this is a good distinction to clarify. Claimable balances are equally ephemeral as accounts on the network, but Horizon provides access to that ephemeral information over time, and any historical data needed to understand transactions and changes in balances should still be accessible. |
Querying operations, transactions, and effects by claimable balance id is not so easy. None of the tables used to store operations, transactions, or effects have a column which could be used to filter by claimable balance id. We would need to add a schema change to be able to support the new claimable balance endpoints @leighmcculloch is proposing. I'm in favor of @paulbellamy 's suggestion of marking the claimable balance as claimed in horizon instead of removing it from the db. Then we could add the extra links that @2opremio proposed which would allow you to locate the creator of the claimable balance and the originating transaction. I acknowledge this behavior differs from the accounts endpoint but we do not have /effects, /operations, and /transactions endpoints on offers either. Furthermore, until CAP 35 is released, the only thing you can do to a claimable balance is claim it where as an account can be mutated in lots of different ways. Having a field in the horizon response makes it really easy for an API consumer to check if the claimable balance was claimed (although I suppose you could argue that inspecting the effects response to check if the balance was claimed is equally easy). |
This is true and the immutability of claimable balances is unlikely to change I think, so they will either be created, or consumed. If we go the route of not adding /operations and /effects to claimable balances we will need a way to signal what has happened to them. Once CAP-35 is merged claimable balances will have one initial state, created, and two terminal states, claimed, or clawed back. On a couple different fronts we've been discussing supporting splitting and joining claimable balances into new claimable balances. If we pursue this, there will be more terminal states, split, and joined_with, and therefore more relationships to navigate. Would all these relationships become new |
Would we have a _links entry for the consuming transaction and operation as well? That would allow us to find out how it was consumed: claimed, clawed back, split, joined, etc. |
I can see one obstacle that will make this idea hard to implement. We rebuild state from time to time because of the ingestion version upgrade. When we rebuild state we use history archives and claimed balances are not present there. This means that to support this feature we would have to reingest entire history (or at least history since claimable balances were added to the protocol) every time we rebuild state. While this is possible at SDF (but will require a huge operational effort) I don't think it'll be easy for our community to do. I said hard not imposible because we can create a separate table where we track deleted claimable balances. However I don't like this option for a single reason: data won't always be consistent between two Horizon instances:
While I was OK with I think this is the perfect example of Horizon API limitations (we can't add every single feature requested by users because we need to support different deployment types) and why we created |
Thanks @bartekn, that is really helpful context. In your last paragraph you point to the ingest package. Are you suggesting that this issue #3305, and #3306 and #3380 not be implemented in Horizon but defer to folks using claimable balances to solve the problem themselves? I see where you're coming from, but I think that will make claimable balances unusable as a primitive for two part peer to peer payments for applications built on Horizon. I might be missing something, please point it out if I am. What changes would be required to claimable balances in the protocol or in core for Horizon to be able to support these features? |
I don't think a protocol change is the right thing to do here: people will want "context" on the claimable balance, at a minimum that would be the transaction that created it (and then maybe ledger information like close time). To me this seems to be pretty much the same problem that people have if they want to know "why" they have a specific balance in their account (because they received a payment or an offer got crossed): the assumption here is that you only get visibility into that information if the Horizon node ingested the relevant range. |
@bartekn that's a good point, I forgot that claimable balances are removed from the history archives once claimed. Given that obstacle, I am no longer in favor of persisting claimed claimable balances in the horizon db. we have talked about serving tx meta directly from horizon as an alternative to effects, see #2175 . I think if we had an endpoint which allowed querying tx meta by I agree with @MonsieurNicolas we shouldn't need to implement a protocol change to fix a shortcoming in horizon |
@bartekn On the other hand, we could just accept that certain features will only be available after the Horizon version supporting them was deployed (and not in prior DB history). It's not ideal, but I think it's better than having a hard requirement on reingesting in order to offer such features.
@tamirms To be clear, you mean not 404'ing anymore when a claimable balance is claimed? That's a breaking change with the current behavior. |
This seems to be converging into:
Regarding (3), it would useful to indicate why and how a claimable balance stopped being claimable. For now it could be due to the claimable balance: a) being claimed I think that, in order to make it future-proof, we could simply point to the transaction and operation which caused the balance not being claimable anymore (e.g. in the future it could be a merging operation). For that I propose: 3.1 Adding an e.g. after being claimed, the claimable balance endpoint should include: {
"available": false
} 3.2 Adding new "_links": {
"self": {
"href": "https://horizon.stellar.org/claimable_balances/00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9"
},
"transaction": {
"href": "https://horizon.stellar.org/transactions/foo",
},
"operation": {
"href": "https://horizon.stellar.org/operations/bar",
},
"reaper_transaction": {
"href": "https://horizon.stellar.org/transactions/blah",
},
"reaper_operation": {
"href": "https://horizon.stellar.org/operations/bleh",
},
}, |
I think what I was trying to suggest is that there are some features (like access to removed ledger entries) that are quite hard to implement in Horizon and you are not limited by anything if you ingest raw ledger data with a little help of Regarding issues:
I checked the required use cases once again and I have some ideas:
I think it would be easier to implement and use if we:
So if you see a claim you can quickly get to the creator (_links -> operation -> source account) and memo (_links -> operation -> transaction -- obviously we can persist tx hash too to save one hop). It will require some changes to the catchup code in Horizon (that phase in which it skips meta) but it's much easier that keeping deleted entries in my opinion. |
I want to add some contextual input from the final protocol 14 working group meeting (June 2020), which was attended by @ire-and-curses @leighmcculloch and myself (among others). Reviewing the notes from that meeting, the plan included:
Obviously, we have learned a lot since then so these plans might not be fully relevant anymore (eg. I think linking to the creating operation and transaction is more valuable than tracking the creating account). But we clearly thought there was value in being able to get the historical operation-level information for a claimable balance entry, and I think this holds true today. |
@jonjove the differentiation is around the phrase "doesn't exist", and if/when a claimable_balance should cease to exist. Intuitively, I still feel like it should never cease to exist, only changing state when claimed. However, given the ingest/implementation issues, @bartekn's suggestion seems like a reasonable solution. |
@bartekn Suggestion sounds good to me, for a solution that doesn't involve adding /transaction, /operations, /effects. As a developer who builds on Horizon I'd still prefer the latter but the former looks like it would address the main use cases. I don't think it covers other possible use cases: |
@bartekn it's not clear to me how your proposal solves the issue. how would you look up the claimable balance details once it has been removed from the db? |
@bartekn asked me for what the use cases are in a DM, so just resurfacing them here:
|
@leighmcculloch we discussed this issue in Horizon meeting yesterday. We think that it should be possible to create a mechanism similar to accounts, which is: for Regarding Does it work for you? |
That sounds good 👍. Questions:
|
Yes. We are planning to do it in a similar way to accounts so there will be a new
Yes. |
Great, sounds great. |
What problem does your feature solve?
There is no effecient method for a receiver (claimant) to identify the sender of a two-part payment, i.e. the creator of a claimable balance, or the memo of the transaction that created it.
It's possible to discover the sender by getting the ledger ID that the claimable balance was created in, and iterating over all operations in that ledger, looking for the operation that created it. This is very inefficient and cumbersome for the developer.
Claimable balances have a sponsor field and that field is set to the account that created the claimable balance if the ledger entry isn't sponsored. However, claimable balances that are sponsored won't have the creator set there and no application should rely on the sponsor being the sender.
Claimable balances were created with the intention of facilitating two-part payments. While they provide generic types for separating an amount into a balance disconnected from any account they still need to facilitate payments like how direct payments do. The sender shouldn't be obfuscated from the receiver, and it should be equally easy to find the memo of the transaction that created it so that if the memo is used as an order ID or a virtual account identifier, it can be looked up.
Example:
https://horizon.stellar.org/claimable_balances/00000000178826fbfe339e1f5c53417c6fedfe2c05e8bec14303143ec46b38981b09c3f9
What would you like to see?
It be possible to view operations relating to the claimable balance via a sub-path
/operations
, similar to how accounts have the same sub-path. A client could use this endpoint to get the first operation relating to a claimable balance, and the first operation will specify who the sender/creator is.It be possible to view the transaction that created the claimable balance, and claimed it.
Similar to how the accounts sub-paths function, the operations sub-path for claimable balances should remain accessible even after a claimable balance is destroyed by being claimed.
What alternatives are there?
We could surface the creator as a first-class field on the claimable balance in Horizon, but it doesn't exist in the network so Horizon would have to fill that data. This would be convenient, however I think exposing a reference to operations would be more useful because with that a client can see not just who created it but in what context.
This issue is related to #3306 and #3380.
cc @stellar/horizon-committers
The text was updated successfully, but these errors were encountered: