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

Continue on "Cannot read property length of undefined" #297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

barrystyle
Copy link

try/catch exception, because things arent meant to stop in PROD..

@uaktags
Copy link
Collaborator

uaktags commented Aug 11, 2019

My only issue with ignoring things like this is that there's a reason we're looking at X for a value, if the value isn't there, then something is wrong. Ignoring and moving on (skipping) transactions or blocks probably isn't what you want to do in PROD either. Instead, the "Cannot read property length of undefined" needs to be investigated for why are we looking for a value that doesn't exist.

@wh1tebeard
Copy link

I found an instance where 'coinbase' had been renamed in the coind output, so it was trying to read an invalid object...

@uaktags
Copy link
Collaborator

uaktags commented Aug 12, 2019

Right, so because you're skipping rather than coding it in, you're missing all of these "coinbase" txes. I don't know why you'd want to do that. Instead, either the coin needs to be fixed or a better fix for the explorer needs to be coded in.

While I'm in agreement that error handling is still lacking, skipping things aren't the answer imo

@barrystyle
Copy link
Author

So the end result is; rather than letting it pass and display results which might have missing information - you want to just have the explorer halt?

@uaktags
Copy link
Collaborator

uaktags commented Aug 12, 2019 via email

@uaktags
Copy link
Collaborator

uaktags commented Aug 12, 2019

I'm confused and struggling to figure out why you would think this is a good idea to do.

You're effectively saying this:
Line 641: if block variable is defined then
Line 642: start the try/catch
Line 643-647: do normal block parsing
Line 648: if any error at all comes up, just skip it.

Any error, not even the coinbase one that the other user brought up. No error message, no logging, can't even troubleshoot it in dev.

So yes, I am saying that the current method of crashing sync.js when a non-compliant coin is introduced that we weren't coded for is WAY better than skipping over the problem.

If you're set on keeping it this way, then keep it as your own fork, but in this PR's current state, no..I don't agree with the changes at all.

Copy link
Author

@barrystyle barrystyle left a comment

Choose a reason for hiding this comment

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

Added an action to print the exception.

@wh1tebeard
Copy link

wh1tebeard commented Aug 14, 2019

My solution was to rewrite sync and database to look for the altered name... I will submit a PR when I have time. Not sure how many people this will affect, but it is something that others may run across, and should be aware of if they are having similar problems. I found the issue by importing util and console.logging the objects that were causing the issue...

@krewshul
Copy link
Contributor

krewshul commented Apr 12, 2020

I think i may be missing the issue. but if this is due to receiving the error Cannot read property length of undefined it is more likely due

  1. the chain hasn't passed the transaction you set in the settings file and just need to let it fully sync. (from my understanding its to ensure wallet is on the right chain.
    or
  2. forgot to use npm install --production

@barrystyle
Copy link
Author

barrystyle commented Apr 13, 2020 via email

@TheHolyRoger TheHolyRoger added this to the Coin-Commands milestone Jul 16, 2020
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.

5 participants