-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use createAsset function from SDK. #73
Conversation
btw, just published an asset on ganache from this branch... all good on my side
|
@mariacarmina Check the chain id on the tests. better also use a try/catch on the caller side... so we know whats happening (as we throw errors) |
Hello @paulo-ocean, if you tested on barge, the chain id from simpleComputeDataset.json is set correctly, maybe the issue was on simpleDownloadDataset.json which is the mumbai chain id, I updated in the code the chain id to see if the tests pass. |
I've seen that within the commands and publish.ts file there are try/catch blocks and they print the errors when they are raised, one spot was missing when publishing the algo, I've added right now, is there any other place that you were referring to? |
The test are still failing can you share how did you receive the chain id error? the tests from CI indicate another type of error. |
@paulo-ocean I have updated the chain ids, now it helped to advance the tests suite, PR to fix the check on ocean.js is merged, I think we need to do a minor release for ocean.js. Thank you! |
Yes, the wrong one was |
We don't get any errors when we code like "throw new Error()" and don't use try/catch :-) |
Right, we also need to check the sdk part, I'm fixing it right now. |
Do you know why the tests are failing? |
we need to fix node modules and package-lock |
better update the
|
@mariacarmina if you can duplicate the
|
If those images do not exists in dockerhub, then we should maintain them up to date, we used to remove those docker images to not load runner resources and to have enough resources to run barge. |
Its not about that... the images exist. Take a break, listen :-)... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes # .
Changes proposed in this PR: