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

CAP-0067 Update #1617

Merged
merged 4 commits into from
Jan 15, 2025
Merged

CAP-0067 Update #1617

merged 4 commits into from
Jan 15, 2025

Conversation

sisuresh
Copy link
Contributor

We should rename SorobanTransactionMeta, but that can be done in a follow up change to this CAP.

@sisuresh sisuresh enabled auto-merge (squash) January 15, 2025 01:12
leighmcculloch
leighmcculloch previously approved these changes Jan 15, 2025
@@ -90,8 +131,9 @@ contract: asset, topics: ["clawback", from:Address, sep0011_asset:String], data:
At the moment, if the issuer is the sender in a Stellar Asset Contract `transfer`, the asset will be minted. If the issuer is the recipient, the asset will be burned. The event emitted in both scenarios, however, is the `transfer` event. This CAP changes that behavior to instead emit the `mint`/`burn` event.

### New Events
This section will go over the semantics of how the additional `transfer` events are emitted for each operation, as well as the `fee` event emitted for the fee paid by the source account. These events will be emitted through `TransactionMeta`, and will not be hashed into the ledger. Note that
the `contract` field for these events corresponds to the Stellar Asset Contract address for the respective asset. Note that the Stellar Asset Contract instance is not required to be deployed for the asset. The events will be published using the reserved contract address regardless of deployment status.
This section will go over the semantics of how the additional `transfer` events are emitted for each operation, as well as the `fee` event emitted for the fee paid by the source account. These events will be emitted through the `events<>` field in `SorobanTransactionMeta`, and the SHA-256 hash of the events will be saved in the new `eventsHash` extension of `TransactionResult`. For consistency, the preimage of the hash stored in `InvokeHostFunctionResult` on success will just be the `returnValue` `SCVal`, and the events hash will be stored in `eventsHash` like any other transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, the preimage of the hash stored in InvokeHostFunctionResult on success will just be the returnValue SCVal

This is undoing a previous change, right? It used to just be the SCVal. Should we revisit why we did that in the first place to make sure we're not missing something by reverting back?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder to post the comments on the discussion thread, especially for more contentious topics.

By using the existing `events<>` vector in `SorobanTransactionMeta`. We can avoid making any xdr changes. This does have some tradeoffs, mainly that
1. All events for a given transaction will be emitted in a single vector, making it impossible to distinguish which operation emitted a specific event. The alternative would be to move Soroban meta from the transaction layer into the operation layer of transaction meta, but that would be a breaking change.
2. Soroban events are hashed into the return value of `InvokeHostFunctionResult` which allows you to cryptographically verify the `events<>` vector. Using the same `events<>` vector may cause some confusion because the Classic events won't be hashed into anything.
By using the existing `events<>` vector in `SorobanTransactionMeta`. We can avoid making any xdr changes. This does have some tradeoffs, mainly that all events for a given transaction will be emitted in a single vector, making it impossible to distinguish which operation emitted a specific event. The alternative would be to move Soroban meta from the transaction layer into the operation layer of transaction meta, but that would be a breaking change.
Copy link
Contributor

Choose a reason for hiding this comment

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

This does have some tradeoffs, mainly that all events for a given transaction will be emitted in a single vector, making it impossible to distinguish which operation emitted a specific event.

This seems... prohibitive to downstream ingesters. Unless the event itself makes it unnecessary to inspect the operation itself (which would require more details in data, imo. Could we make events<> a nested vector, instead, in a binary-compatible way, or will that not work because of a prepended length byte? I'm not 100% sure how XDR encodes things.

@sisuresh sisuresh merged commit 5fa1220 into stellar:master Jan 15, 2025
3 checks passed
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.

4 participants