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

Problem: Adding chain-maind keys will not be actually stored in disk … #441

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

leejw51crypto
Copy link
Contributor

Solution: add decoding _hexcode in service name fetched

@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #441 (d0a044a) into master (f8f0ba5) will increase coverage by 3.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   17.00%   20.20%   +3.20%     
==========================================
  Files          55       56       +1     
  Lines        8399     8768     +369     
==========================================
+ Hits         1428     1772     +344     
- Misses       6572     6574       +2     
- Partials      399      422      +23     
Flag Coverage Δ
integration_tests 17.98% <ø> (+0.98%) ⬆️
unit_tests 11.48% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/app.go 18.38% <0.00%> (-81.62%) ⬇️
x/subscription/types/codec.go 0.00% <0.00%> (-5.89%) ⬇️
x/supply/client/rest/query.go 3.40% <0.00%> (-4.60%) ⬇️
x/subscription/types/msgs.go 5.79% <0.00%> (-1.82%) ⬇️
app/test_helpers.go 0.00% <0.00%> (ø)
x/subscription/types/params.go 0.00% <0.00%> (ø)
x/chainmain/types/types.go
x/chainmain/types/keys.go 65.62% <0.00%> (ø)
config/config.go 55.55% <0.00%> (ø)
x/subscription/keeper/msg_server.go 32.03% <0.00%> (+0.81%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8e5245...d0a044a. Read the comment docs.

@leejw51crypto
Copy link
Contributor Author

leejw51crypto commented Mar 31, 2021

keyring pr is here
https://github.com/99designs/keyring/pull/83/files

keyring is stored here in linux)
$HOME/.local/share/keyrings

@leejw51crypto leejw51crypto force-pushed the feature/437 branch 3 times, most recently from b7cba23 to 02c977e Compare March 31, 2021 12:48
@JayT106
Copy link
Contributor

JayT106 commented Mar 31, 2021

I see some of the modules been removed from the go.sum. How do they relate to the fixes?
For the keyring dependency, is it better to merge @leejw51crypto 's fixes to the upstream repo first and then use it? (Not sure how urgent is the fixes)

@tomtau
Copy link
Contributor

tomtau commented Apr 1, 2021

yeah, may be better to wait for the upstream -- it seems it's maintained

@leejw51crypto
Copy link
Contributor Author

ok, got it
because it's simple fix, can be merged shortly.

@leejw51crypto
Copy link
Contributor Author

leejw51crypto commented Apr 1, 2021

I see some of the modules been removed from the go.sum. How do they relate to the fixes?
For the keyring dependency, is it better to merge @leejw51crypto 's fixes to the upstream repo first and then use it? (Not sure how urgent is the fixes)

i ran "go mod tidy" to check mod

this is urgent issue, because in linux, keyring is not usable by default.
users of linux cli will encounter this issue.

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

there are some conflicts with the latest master.

it seems upstream may take time -- I've forked the repo with the fix here, so the go.mod can be updated to this one: https://github.com/crypto-org-chain/keyring/tree/gnome for the moment

@leejw51crypto
Copy link
Contributor Author

leejw51crypto commented Apr 20, 2021

ok, i'll rebase with the latest
also, writing unit test for keyring

go.mod Outdated Show resolved Hide resolved
@leejw51crypto leejw51crypto force-pushed the feature/437 branch 3 times, most recently from 50d184d to 39150a1 Compare April 20, 2021 12:34
@leejw51crypto
Copy link
Contributor Author

inspecting integration test failures

@leejw51crypto
Copy link
Contributor Author

fixed, increase waiting time twice :-)

…ix crypto-org-chain#437)

update vendorsha256

Update go.mod

thanks

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>

go mod tidy

add todo

increase time

fix lint issue

replace jwt-go

update nix

fix gomod2nix

restore gomod2nix

remove vendorPath
@tomtau tomtau self-requested a review April 22, 2021 01:40
@tomtau tomtau merged commit 4286599 into crypto-org-chain:master Apr 22, 2021
Copy link

@matias904 matias904 left a comment

Choose a reason for hiding this comment

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

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

5 participants