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

Integrate wallet providers #65

Merged
merged 33 commits into from
Nov 14, 2019
Merged

Integrate wallet providers #65

merged 33 commits into from
Nov 14, 2019

Conversation

corollari
Copy link
Collaborator

Integrates NEOLine, enabling the use of the account stored in the extension to deploy and invoke smart contracts.
Currently deploying is not working due to what I believe to be issues with NEOLine itself, I've notified upstream and once issues are fixed there this PR should work fine.

Posting it here so that if other people try to implement it they will see this and won't have to redo the work I've done.

If you believe this should only be posted once it's ready to be merged feel free to close it directly.

@vncoelho
Copy link
Member

vncoelho commented Jul 10, 2019

You are more than welcome, @collari.
That is a great work!

We can give you permission to direct edit here as well if you want.

I will test the PR and install NeoLine here as well.
We could also update the Readme with a guidelines for NeoLine, yes?

@vncoelho vncoelho marked this pull request as ready for review July 10, 2019 16:28
public/js/eco-scripts/invoke_deploy_NeonJS.js Outdated Show resolved Hide resolved
public/js/eco-scripts/invoke_deploy_NeonJS.js Outdated Show resolved Hide resolved
public/partials/ecolab.html Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

Thanks for the integration, @corollari,

I do not know why but our github does not show the button to upon with master. Thus, there are some duplicated things from the previous commit.

I left some minor comments, let's finish this integration, it will be nice!

@corollari
Copy link
Collaborator Author

The last commit addresses the changes you requested (clean the code, re-organize it into functions to reduce duplication and change the network selection UI) and extend the integration to O3 and NeoLogin.

Full disclosure: I'm the lead dev of NeoLogin

@corollari corollari changed the title [WIP] Integrate NEOLine Integrate wallet providers Oct 13, 2019
@corollari
Copy link
Collaborator Author

Pushed an update that shows the wallets only if they are available (user has them installed). Before they always showed up but if a user tried to select them but they didn't have them installed an error would pop up.

If in your opinion it was better before I can just revert it.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Great job, @corollari.

I still need to test. I am just checking the code for now.

I believe it might be almost ready.

public/js/eco-scripts/abi_manager.js Outdated Show resolved Hide resolved
public/js/eco-scripts/invoke_deploy_NeonJS.js Outdated Show resolved Hide resolved
public/partials/ecolab.html Show resolved Hide resolved
public/partials/ecolab.html Show resolved Hide resolved
@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2019

@corollari, I do not know why, but here in our repo I do not see how to update your PR based on master.

Do you know how to fix that?

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2019

"ERROR (DEPLOY): Only MainNet and TestNet are supported by wallet providers."

I tried on testnet but it says:
"Error: Connection destroyed
at neologin.js:8
at neologin.js:8
at Array.forEach ()
at destroy (neologin.js:8)
at neologin.js:8"

@corollari, it would be nice if the wallet of NEOLOGIN was added to the http://localhost:8000/#!/ecolab/wallet tab. Thus, the user can easily the balance it has.

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2019

During deploy:

    Uncaught (in promise) ReferenceError: idToDeploy is not defined
    at handleDeploy (invoke_deploy_NeonJS.js:297)
    at invoke_deploy_NeonJS.js:284"

@corollari
Copy link
Collaborator Author

No idea how to fix the thing with github but I just rebased the PR on master, that should have the same effect.

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2019

Perfect, I had mad a PR on your repo for such update. But the rebase may had solved that.

@@ -3052,4 +3083,6 @@ <h4>ABI:</h4>
sendingTxPromiseWithEcoRaw(PendingTX,PendingTXParams);
}
</script>
<script src="https://cdn.jsdelivr.net/npm/neo-dapi@1.0.2/lib/neo-dapi.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

before merging we should download these .js and load from the file.
We are doing that with all packages of neocompiler in order to avoid dependencies with external providers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! I've also added instructions on how to download these packages (especially important in the case of O3 because versions of the js package which are newer than 1.0.2 do not work) to public/js/README.md

@corollari
Copy link
Collaborator Author

It seems github automatically marked your PR as merged

@corollari
Copy link
Collaborator Author

Working on solving the problems that you raised on deploy() currently

@corollari
Copy link
Collaborator Author

Just fixed the following error:

I tried on testnet but it says:
"Error: Connection destroyed
at neologin.js:8
at neologin.js:8
at Array.forEach ()
at destroy (neologin.js:8)
at neologin.js:8"

@corollari
Copy link
Collaborator Author

corollari commented Nov 1, 2019

Will start working on adding addresses & balances to the wallet tab

@corollari
Copy link
Collaborator Author

Just fixed the bug that caused the following error you reported:

    Uncaught (in promise) ReferenceError: idToDeploy is not defined
    at handleDeploy (invoke_deploy_NeonJS.js:297)
    at invoke_deploy_NeonJS.js:284"

@corollari
Copy link
Collaborator Author

@corollari, it would be nice if the wallet of NEOLOGIN was added to the http://localhost:8000/#!/ecolab/wallet tab. Thus, the user can easily the balance it has.

Just pushed one last commit that makes the wallet provider's address appear in all the places where the set of pre-defined addresses of ECO_WALLET usually appear, including the /wallet tab. Thoughts?

@vncoelho
Copy link
Member

vncoelho commented Nov 4, 2019

@corollari, Alllbert, do you have O3 installed and running, right?

Take a look at this api: https://neodapitestbed.o3.app/index.html
When you change network and wallet on the O3 interface the address also changes on the front-end (their api was not complete, but I think it gives a good idea. Andreit sent it so me sometime ago). I believe that we can reach an even better level here on NeoCompiler.

For the neocompiler-eco we would need to update the ECO_WALLET' array.

@vncoelho
Copy link
Member

vncoelho commented Nov 6, 2019

@corollari, can you give me access to your branch?

image

@vncoelho
Copy link
Member

vncoelho commented Nov 6, 2019

@corollari, I changed the provider from "None" to "NeoCompiler-Eco".

We still need to update the other parts that use the flag "None"

@corollari
Copy link
Collaborator Author

corollari commented Nov 7, 2019

@vncoelho When I created the PR I ticked the checkbox to give you access to the branch but maybe there's smth wrong, do you want me to port these commits into a branch of the neoresearch repo?

@vncoelho
Copy link
Member

vncoelho commented Nov 7, 2019

In fact, it is strange, I was able to commit, but not to merge the PR corollari#2.

I think it is ok, @corollari. I will work on the PR locally and try to push.

@vncoelho
Copy link
Member

vncoelho commented Nov 8, 2019

I have added the code to get the account on neocompiler-eco to change when it is changed in the wallet but I can't get it to work.

This should work.
Another good example would be the network.
If the network is changed in the wallet provider it should change on neocompiler as well.

@corollari
Copy link
Collaborator Author

Is there anything missing on my end? I'm not trying to put pressure but just to make sure that I haven't forgotten something due to the conversation being split between discord and github.

@vncoelho
Copy link
Member

Noo, @corollari, you are doing great.
The delay in merging is my fault now. I am trying to give priority on this.

But we focused on removing all node requirements from the project. Now, everything is dockerized! This helped a lot.
Tomorrow I will surely try to run it.

}
if($("#ecolabproviderselection").val() === "NeoCompiler-Eco"){
changeAccount(window.storedECO_WALLET);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is not working. I do not know why but the EcoWallet is being kept as the original.

Copy link
Member

Choose a reason for hiding this comment

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

It was not working on Brave Browser!
On firefox it worked.

Copy link
Member

Choose a reason for hiding this comment

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

It is strange, @corollari, because https://neologin.io/testbed/#collapse-getProvider works on Brave.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's really strange because I did all my development on Chromium.. did you manage to find the root cause?

Copy link
Member

Choose a reason for hiding this comment

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

neologin-1.2.4.js:8 Uncaught (in promise) Error: Failed to read the 'localStorage' property from 'Window': Access is denied for this document.
    at main.17a8c4b5.chunk.js:1
    at new Promise (<anonymous>)
    at X (main.17a8c4b5.chunk.js:1)
    at Object.keys.map._.<computed> [as getAccount] (main.17a8c4b5.chunk.js:1)
    at 2.6d2d096a.chunk.js:1
    at new Promise (<anonymous>)
    at l (2.6d2d096a.chunk.js:1)

I think that the error is this one.
It keep Pending on GetAccount...I do not know. Strange.But on firefox it is ok.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I was able to test everything now, @corollari!!!
I think that we are almost there.

Pending things are:

  1. Track the changes on EcoWallet on every step, thus, we do not lose them when we move to Provider and come back;
  2. Enable transfers. Otherwise we can not claim GAS from the NeoCompiler-Eco Front-end.
  3. Remove the checkProviderNetwork check, why we can not operate on PrivateNet or SharedPrivateNet? We just need NeoLogin capabilities for signing the txs, right?

I will also try to help on them tomorrow, perhaps I can tackle 1.

@vncoelho vncoelho changed the base branch from master to neocompiler-eco-neo3-preview November 11, 2019 13:24
@vncoelho vncoelho changed the base branch from neocompiler-eco-neo3-preview to master November 11, 2019 13:24
@corollari
Copy link
Collaborator Author

On it!

@vncoelho
Copy link
Member

@corollari, great job, once more!
I think we can merge it. All standard functionalities look like to be working perfect!

@vncoelho vncoelho merged commit 5b30afd into NeoResearch:master Nov 14, 2019
@vncoelho
Copy link
Member

vncoelho commented Nov 14, 2019

@corollari , thanks for everything.

I did not double check, quite busy here with Wang Yong Qiang and some talks on some Universities here..ahauah Great time!
But we manage future feature and improvements on next PRs.

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.

None yet

2 participants