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

Complete work on eth-event-viewer #2

Open
wants to merge 79 commits into
base: initial-commit-branch
Choose a base branch
from

Conversation

dominic22
Copy link
Owner

… on-agent to handle the eth-watcher response
…se abi result, add eth-watcher helper functions
Copy link

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

This certainly fulfills the bounty! But that's not what I'm here for. I'm here for the strict code review. (;

I tried and review the hoon here pretty thoroughly. Only really skimmed the js, but didn't see anything too crazy there. Let me know if there's anything specific there you want feedback on, and I can take a look.

Usability things:

I know we've already spent some time talking about eth-watchers idiosyncrasies. Arguably it needs a "release after x blocks" parameter. Until that's fixed, we'll just have to live with the slightly-slower release.

Would be nice if this also read from the data.event-log field and rendered those in the event lists too. It already loads in full event signatures, so should be able to do that, right?

The hardcoded "from" block should probably change. Suggestion: on the backend, just take in a block number alongside the contract address/config. On the frontend, render a slider, or numerical input or w/e, for selecting between "starting now", "from last week", etc, "from contract creation". I think it can get the contract creation date & current block number from the Etherscan API, right? Actually, all API calls here go through the backend, it seems? Probably wouldn't want to add frontend-side calls just for that...

General code things:

The order of the arms in your app core is kinda unconventional. Generally best to group the lifecycle ones (init, save, load) together at the top, then follow with request (poke, watch) and result (agent, arvo, fail) ones. That generally mimics execution orders/flows more tightly, making for a slightly comfier read-through. (But I'm probably making it sound like a bigger deal than it is.)

Also a bunch of comments below.

urbit/app/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/app/eth-event-viewer.hoon Show resolved Hide resolved
urbit/app/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/app/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/app/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/sur/eth-event-viewer.hoon Show resolved Hide resolved
urbit/sur/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/mar/eth-event-viewer.hoon Outdated Show resolved Hide resolved
urbit/lib/eth-watcher.hoon Outdated Show resolved Hide resolved
urbit/lib/eth-event-viewer.hoon Show resolved Hide resolved
@dominic22
Copy link
Owner Author

@Fang- I've worked through your suggestions and also added the request of the block number, so the user does not have to insert it by himself. Furthermore the user is able to request the block number of the prev. week by clicking a button. I've also made the ui more robust, since the user can run into "Max rate limit reached, please use API Key for higher rate limit" when requesting the etherscan api too often in a short time period.

Please have another look at the app if it fits your expectations and please let me know if the bounty can be completed with my revised version. :)

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.

2 participants