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

VIP Structs in Events #1170

Closed
fubuloubu opened this issue Jan 3, 2019 · 6 comments · Fixed by #2403
Closed

VIP Structs in Events #1170

fubuloubu opened this issue Jan 3, 2019 · 6 comments · Fixed by #2403
Labels
VIP: Approved VIP Approved

Comments

@fubuloubu
Copy link
Member

fubuloubu commented Jan 3, 2019

Simple Summary

Allow emitting structs in events

Abstract

ABIv2 will allow emitting structs inside events.
This behavior is not well-defined (@davesque please confirm).

Motivation

Working with structs as input arguments, it may be useful to emit structs inside events for the client-side to use after a transaction has been mined e.g. a Deposit transaction in Plasma.

As mentioned by @charles-cooper in #1019

Specification

Specification is WIP, but here are a few possibilities:

Directly outputing a struct:

struct Transaction:
    prevBlkNum: uint256
    tokenId: uint256
    newOwner: address
    signature: bytes[65]  # VRS

DepositAdded: event({txn: Transaction})

def foo(txn: Transaction):
    log.DepositAdded(txn)

Outputting only certain members of a struct:

# Should we allow indexing event args in this way?
DepositAdded: event({tokenId: uint256, newOwner: address})

def foo(txn: Transaction):
    # Compiles to `log.DepositAdded(txn.tokenId, txn.newOwner)`
    log.DepositAdded(**txn)  # because who doesn't love kwargs?

Indexing:

# Do we allow structs to be indexed? ABIv2 is inconclusive I believe
DepositAdded: event({txn: indexed(Transaction)})

# Should we allow indexing event args in this way?
DepositAdded: event({tokenId: indexed(uint256), newOwner: address})

# Should we allow indexing members indirectly?
DepositAdded: event({txn: Transaction}, indexed=[txn.newOwner, txn.tokenId])

Backwards Compatibility

No backwards compatibility issues

Copyright

Copyright and related rights waived via CC0

@fubuloubu fubuloubu mentioned this issue Jan 3, 2019
4 tasks
@davesque
Copy link
Contributor

davesque commented Jan 4, 2019

@fubuloubu I believe that information included in an event is encoded as arguments of a functional call would be minus any function selector at the beginning. This would include structs which would be encoded as tuples. Source is second sentence in this section of the current ABI spec: https://solidity.readthedocs.io/en/develop/abi-spec.html#argument-encoding

@charles-cooper
Copy link
Member

charles-cooper commented Jan 4, 2019

@fubuloubu I like the syntax for directly outputting a struct but I think the member selection is too magical. Users should have to specify which fields they want to log/index.

Re. indexing on a struct, according the EVM spec, each LOG topic is one word, so we would have to apply a map from struct to uint256/bytes32 (e.g. keccak256) and I think that is overfunctioning on behalf of the user. Besides, the topic data is not accessible so I'm having trouble thinking of a case where somebody would want to index a struct but not already have the hash.

I think Solidity uses the ABIv2 encoding but I'm not sure if it is packed or unpacked. For compatibility we should use whatever it uses.

@jacqueswww jacqueswww added the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Jan 6, 2019
@charles-cooper
Copy link
Member

According to solidity team https://gitter.im/ethereum/solidity-dev?at=5c3328241948ad07e8000c16

non-indexed structs should be abi-encoded, indexed structs should be packed and hashed.

@fubuloubu
Copy link
Member Author

"packed and hashed" hmm. So is it included both as a hash and as an ABI-encoded, unindexed data?

@charles-cooper
Copy link
Member

Not automatically. If you wanted to do that you would have to include it twice, like event Log(indexed Foo foo, Foo foo). It seems cumbersome but since logging costs a decent amount of gas the programmer should have control over those things.

@fubuloubu
Copy link
Member Author

I guess it would be helpful if the struct was a transaction, and you were waiting on it to be confirmed in the contract, sort of like "transaction hash was mined!"

@fubuloubu fubuloubu added this to To do in v1.0 Release Candidate via automation Oct 24, 2019
@fubuloubu fubuloubu added VIP: Approved VIP Approved and removed VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting labels Jan 6, 2020
@fubuloubu fubuloubu added this to the v0.3.0 Release milestone Jul 6, 2020
v1.0 Release Candidate automation moved this from To do to Done Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved
Projects
Development

Successfully merging a pull request may close this issue.

4 participants