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

Remove block numbers from the smart contract events #450

Closed
hackaugusto opened this Issue Mar 23, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@hackaugusto
Collaborator

hackaugusto commented Mar 23, 2017

Problem Definition

NettingChannel expose a few events with the block number, that value is duplicated since it's available through the eth_getFilterChanges

blockNumber: QUANTITY - the block number where this log was in. null when its pending. null when its pending log.

Solution

Remove the block_number fields from all smart contract event and use the result from the JSON RPC call.

Tasklist

  • remove the block_number
    • ChannelNewBalance
    • ChannelClosed
    • TransferUpdated
    • ChannelSettled
  • Expose the blockNumber from the log to the filter changes
  • Fix the code that needs the block number.
@hrishikeshio

This comment has been minimized.

Contributor

hrishikeshio commented Mar 29, 2017

I have done the changes. But can you explain "Fix the code that needs the block number"?

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Mar 29, 2017

@hrishikeshio
There are places in the events handling where the block number is used by reading the event argument.
Instead we can use the JSON RPC call for that. That is what

Fix the code that needs the block number.

means.

konradkonrad added a commit to konradkonrad/raiden that referenced this issue Jul 14, 2017

Set all event block numbers to `0`
Since the event fields for `block_number` are redundant (see raiden-network#450) -- we
need to discourage their use. By setting them to a non-functional value,
we can ensure, their usage will raise issues.
@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Jul 30, 2017

I'm planning to work on that, but a quick search showed block_number instance attribute of diverse events being heavily used on the code. As it may be unnecessary to retrieve the block_number from the API every time it is required, maybe we could keep the attribute, but standardize it's use, maybe with a superclass for all events, and it's fetching, from eth_getFilterChanges (as I started this work in #836).

@andrevmatos andrevmatos self-assigned this Jul 31, 2017

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Aug 28, 2017

Instead of removing the block_number and adapting every code that used it to re-fetch the event block number, we standardized populating it for every event that had this attribute, getting it directly from events handling/polling. Through #902 and #836 , I'd consider it as solved.

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Aug 28, 2017

Sure, it can be considered closed.

@LefterisJP LefterisJP closed this Aug 28, 2017

@hackaugusto

This comment has been minimized.

Collaborator

hackaugusto commented Aug 29, 2017

How is this solved? The event's block_number was not removed.

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Aug 29, 2017

@hackaugusto look at my last comment above: instead of removing it, we standardized it's fetching from the logs ( eth_getLogs|FilterChanges), and with that, avoided the need to provide it in the constructor, while keeping the attribute, used in numerous places.

@hackaugusto

This comment has been minimized.

Collaborator

hackaugusto commented Aug 29, 2017

That is the point of the issue, use the block number from the eth_getLogs and then remove the attribute

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Aug 29, 2017

@hackaugusto Just to clarify, are you talking about the block number attribute from

  1. the on-chain events;
  2. the event objects in Raiden;

or both?

@hackaugusto

This comment has been minimized.

Collaborator

hackaugusto commented Aug 29, 2017

The on-chain events that are listed on the task list, it doesn't make sense to remove the block number from the Raiden events.

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Aug 29, 2017

Hmm, right.. I confused it on the description with the Python objects that represented the events, as they too had the blocknumber passed in constructor (which was already fixed), but yes, the on-chain events are not properly fixed yet. Re-opening until I can look at the contracts.

@andrevmatos andrevmatos reopened this Aug 29, 2017

@LefterisJP LefterisJP added this to the Next minor release milestone Sep 14, 2017

@LefterisJP LefterisJP added the 3 label Sep 14, 2017

andrevmatos added a commit to andrevmatos/raiden that referenced this issue Sep 18, 2017

andrevmatos added a commit to andrevmatos/raiden that referenced this issue Sep 18, 2017

andrevmatos added a commit to andrevmatos/raiden that referenced this issue Sep 18, 2017

hackaugusto added a commit that referenced this issue Sep 18, 2017

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Sep 18, 2017

For the records: this issue was solved by:
#836 : first patch to populate event's block_number through blockNumber in which the event was mined
#902 : improvements on above PR, to fetch events with a single RPC call
#945 : finally removed the block_number events attribute, also fetching the block_number on eth_getFilterChanges calls (events polling).

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