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

Event Names Should not be Human Readable #53

Open
villanuevawill opened this issue Nov 26, 2018 · 6 comments
Open

Event Names Should not be Human Readable #53

villanuevawill opened this issue Nov 26, 2018 · 6 comments

Comments

@villanuevawill
Copy link

In the new version of std bounties 2.0, there needs to be an easy way to listen/index events that occur. In std bounties 1.0, this was simple. You could just listen to the deployed contract. This no longer is simple since the new PROXY architecture creates a new contract for every instance of a bounty. In turn, listening for an event does not become so easy (unless you specifically want to listen to the event from an array of ALL addresses in the current factory). Listening for all addresses becomes problematic and requires consistently checking for any new addresses to listen to. As more and more bounties are created, this approach becomes less and less scaleable.

Proposed solution:

The event should not be human readable. This is because a SHA Hash is created from the name of each event along with its argument types. This topic or hash can be used to index/listen for the event theme on chain. However, our event names could easily intersect with other events on other contracts. In order to prevent this confusion, non-human readable event names should be created so we can listen to the event and know it comes from std bounties 2.0

Additionally, topics are created for each argument (first 3) passed into the event. To allow for more flexibility on whoever is listening to the various contracts, we should make sure all events include the ID of the bounty contract in relation to the contract factory. This makes it so you can easily listen to events by ID without having to know the address.

cc: @mbeylin @c-o-l-o-r

@villanuevawill
Copy link
Author

@mbeylin another thing I missed. The 1st argument should actually be the master copy address (that way we can index by specific factory versions). The ID of the bounty or address of the proxy contract itself should be passed as the 2nd argument.

@mbeylin
Copy link
Collaborator

mbeylin commented Nov 27, 2018

@villanuevawill wouldn't it be possible to just get the emitting address (i.e. the proxy address) from the event itself?

re: the master copy address, wouldn't we also be able to get this at the time of creating the bounty contract? (we're planning on removing the ability to change the master copy address, so the result becomes that the mapping of proxyAddr->masterCopy isn't mutable)

@mbeylin
Copy link
Collaborator

mbeylin commented Nov 27, 2018

I also want to push back a bit on having the events be non-human readable, which IMO kind of sucks and I'm not sure it's necessary. Couldn't the flow be:

  1. receive event with SHA hash that matches one of ours
  2. check if the emitter of the event is one of our bounties
  3. if yes, proceed, if no, ignore it

Step 2 is really the only part that might "not scale", but in reality I can't see this ever feasibly causing issues for us. Curious to hear your thoughts @villanuevawill

@mbeylin
Copy link
Collaborator

mbeylin commented Nov 27, 2018

Another solution could just be to prefix all of our events with StandardBounties, i.e. StandardBountiesFulfillmentAccepted, which greatly decreases the chances of us ever running into a collision with the events from another contract. In my opinion the issue still persists (this is just security by obfuscation), but might help?

@villanuevawill
Copy link
Author

@mbeylin prefacing with StandardBounties is ok and solves the problem.
We need the proxy address as one of the first three arguments in the event so events become indexable by their event names and the master address. This is important because our syncing infrastructure will likely only be interested in certain master contracts. If someone wants to launch their own std bounties 2.0 setup with their own factory, we do not want to by default read their events when scanning for bounties related to our master contract. Does that make sense?

@villanuevawill
Copy link
Author

By adding the master contract address as an argument in the events, we can now search for all events related to the master contract. (This also makes sure we don't get collisions and we don't have to do extra extraneous steps like make sure it belongs to the master address). This is the most scaleable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants