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

Add keplr wallet to help page #154

Closed
wants to merge 9 commits into from

Conversation

giansalex
Copy link
Contributor

@giansalex giansalex commented May 2, 2022

Related w/ chainapsis/keplr-wallet#392
Depends on #275

@jaekwon
Copy link
Contributor

jaekwon commented May 3, 2022

Awesome I will get to it asap!

@giansalex
Copy link
Contributor Author

giansalex commented May 4, 2022

we need to enable cors in rpc gno.land:36657 to query account info.

@giansalex
Copy link
Contributor Author

I added a /txs endpoint, and it works

image

image

@moul
Copy link
Member

moul commented May 9, 2022

@giansalex, what's the current step? do you need a review/final review or do you want some more time to make changes? (the PR is in draft mode)

@giansalex
Copy link
Contributor Author

Hey @moul, code is ready, but it will be necessary to enable CORS in gno.land:36657

I also sent a pull request to keplr repo, chainapsis/keplr-wallet#392

Here is a guide to install the dev version.
https://gno.land/r/boards:disperze/61

@giansalex giansalex marked this pull request as ready for review May 18, 2022 23:18
@giansalex
Copy link
Contributor Author

giansalex commented May 19, 2022

some pending issues (for keplr pr).

  • Fee is currently a constant (1gnot)
  • It is necessary to implement a Tx indexer to search txs

@moul
Copy link
Member

moul commented May 19, 2022

Hey @moul, code is ready

That's awesome @giansalex 👍👍👍

it will be necessary to enable CORS in gno.land:36657

Can you help list the CORS restrictions we can add, i.e., which headers, methods, origins, etc?

It is necessary to implement a Tx indexer to search txs

Are you okay that we merge your PR and open an issue with all the ongoing things to do, or do you prefer to make an atomic PR with everything inside?

Here is a guide to install the dev version. gno.land/r/boards:disperze/61

I suggest:

if you don't do it, I'm sure someone else will, so no problem :)


Once again, congratulations for your work!

@moul moul requested review from moul and jaekwon May 19, 2022 08:22
chainId: chainId,
chainName: "GNO Testnet",
rpc: 'http://gno.land:36657',
rest: 'https://lcd.gno.tools',
Copy link
Member

Choose a reason for hiding this comment

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

can you add a link to the repo in a comment here?

Copy link
Member

Choose a reason for hiding this comment

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

@jaekwon what do you prefer between:

  1. letting this external service dependency
  2. hosting an official one under the gno.land domain
  3. extending the gnoland server to support an additional REST endpoint here

I suggest 1 for now, and 3 as the target.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rest: 'https://lcd.gno.tools',
rest: 'https://lcd.gno.tools', // https://github.com/disperze/gno-api

should be this if I'm not wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that api is only for Keplr, but I think Keplr will only use the RPC in the future.

const gnoToken = {
coinDenom: "GNOT",
coinMinimalDenom: "gnot",
coinDecimals: 6,
Copy link
Member

Choose a reason for hiding this comment

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

TIL :)

where is this number coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from ATOM, I'm not sure if Jae wants to change that.

gnoland/website/main.go Outdated Show resolved Hide resolved
gnoland/website/main.go Outdated Show resolved Hide resolved
gnoland/website/main.go Outdated Show resolved Hide resolved
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

I've finished a first round of reviews

Most of them are more about cosmetics and could be skipped and done later if there were an urge to merge it

@moul
Copy link
Member

moul commented May 19, 2022

About my question about CORS config, can we go with this:
disperze/gno-api@a9dd611#diff-c444f711e9191b53952edb65bfd8c644419fc7695c62611dc0fb304b4fb197d6R37-R41

	c := cors.New(cors.Options{
		AllowedOrigins: []string{"*"},
		AllowedMethods: []string{http.MethodGet, http.MethodPost},
		AllowedHeaders: []string{"Content-Type", "Accept"},
	})

@giansalex
Copy link
Contributor Author

giansalex commented May 19, 2022

it is only need to enable cors_allowed_origins in the rpc config.

[rpc]

# TCP or UNIX socket address for the RPC server to listen on
laddr = "tcp://127.0.0.1:26657"

# A list of origins a cross-domain request can be executed from
# Default value '[]' disables cors support
# Use '["*"]' to allow any origin
cors_allowed_origins = ["*"]

@moul
Copy link
Member

moul commented May 20, 2022

Wow, well done for the pb/rpc bridge

Can you add the generate instructions in the Makefile?

@giansalex
Copy link
Contributor Author

ok, i will add it.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

@jaekwon, from my POV -> LGTM.

Can you give a second look, and reply to the various questions, especially the one about lcd.gno.tools?

@moul
Copy link
Member

moul commented Jul 9, 2022

FYI, we’ve added CORS headers on staging.gno.land and soon on test2.gno.land

Can it allows you to remove your proxy?

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Just updating the PR status while we address the last details

@moul moul marked this pull request as draft July 9, 2022 02:30
@giansalex
Copy link
Contributor Author

this issue is necessary #275

@moul moul added this to the 💡Someday/Maybe milestone Oct 20, 2022
@giansalex
Copy link
Contributor Author

With adena as the main wallet, this is obsolete.

@giansalex giansalex closed this Dec 9, 2022
@moul
Copy link
Member

moul commented Jun 29, 2023

It still makes sense to add Gno support on Keplr. Feel free to reopen whenever it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants