-
Notifications
You must be signed in to change notification settings - Fork 58
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 dependency to eoscanada endpoint #151
Conversation
sduchesneau
commented
Apr 24, 2020
- Set default API URL to http://localhost:8888,
- add support for header in environment variable EOSC_GLOBAL_HTTP_HEADER_X (where X is between [0,25])
- adapt README
… var, adapt README
README.md
Outdated
|
||
Read more below for details. | ||
## Environment variables | ||
--------------------- |
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.
This line is a left over no?
README-cn.md
Outdated
@@ -21,6 +21,7 @@ | |||
|
|||
```bash | |||
go get -u -v github.com/eoscanada/eosc/eosc | |||
|
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.
Not sure of the need for a blank line.
README.md
Outdated
@@ -344,9 +300,23 @@ need to stitch transaction files, or gather signatures into a single | |||
place. | |||
|
|||
|
|||
## Cryptographic primitives used | |||
----------------------------- |
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.
Leftover.
eosc/cmd/root.go
Outdated
@@ -47,7 +47,7 @@ func init() { | |||
RootCmd.PersistentFlags().BoolP("debug", "", false, "Enables verbose API debug messages") | |||
RootCmd.PersistentFlags().StringP("vault-file", "", "./eosc-vault.json", "Wallet file that contains encrypted key material") | |||
RootCmd.PersistentFlags().StringSliceP("wallet-url", "", []string{}, "Base URL to wallet endpoint. You can pass this multiple times to use the multi-signer (will use each wallet to sign multi-sig transactions).") | |||
RootCmd.PersistentFlags().StringP("api-url", "u", "https://mainnet.eoscanada.com", "API endpoint of eos.io blockchain node") | |||
RootCmd.PersistentFlags().StringP("api-url", "u", "http://localhost:8888", "API endpoint of eos.io blockchain node") |
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.
Frankly, I'm far from seeing this as a good move. This makes the out of the box experience much to be desire. What about asking EOS Nation of Greymass if they would agree that their API is the default value?
I would even prefer no value at all and a good error message that explains how to properly setup eosc
.
Here what would look like a default get_info
invocation:
eosc get info
ERROR: get info: http://localhost:8888/v1/chain/get_info: Post http://localhost:8888/v1/chain/get_info: dial tcp [::1]:8888: connect: connection refused
It's not really helpful and super hard to understand what needs to be modified exactly. I get the idea of using localhost:8888
, but I don't like it.
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.
eosc is not only for EOS mainnet, therefore having another API provider does not seem like a good solution either. I can add a good error message!
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.
Of course it's usable with all other chains, but still, I bet most usages are made against EOS Mainnet.
Wallet usually list EOS Mainnet as their primary option. They also do not have the exact same problem since there is a bootstrap phase that ask to configure the wallet correctly.
We should probably aim to that kind of workflow also at some point.
Anyway, I don't think I will change the decision, it's too late I guess.
A better error message is the minimum, hopefully it does not trickle down too much everywhere.