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

[BREAKING] client-sdk: Fix ETH address format for test accounts #1352

Merged
merged 1 commit into from May 8, 2023

Conversation

matevz
Copy link
Member

@matevz matevz commented Apr 27, 2023

Fix ETH address handling for non-secp256k1 test accounts.

Fixes oasisprotocol/cli#63

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #1352 (8218476) into main (a49be07) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage   56.14%   56.14%           
=======================================
  Files         120      120           
  Lines        8522     8522           
=======================================
  Hits         4785     4785           
  Misses       3700     3700           
  Partials       37       37           
Impacted Files Coverage Δ
client-sdk/go/helpers/address.go 100.00% <100.00%> (ø)
client-sdk/go/testing/testing.go 95.23% <100.00%> (ø)

@@ -58,7 +58,11 @@ func ResolveAddress(net *config.Network, address string) (*types.Address, *ethCo
case addressExplicitTest:
// Test key.
if testKey, ok := testing.TestAccounts[data]; ok {
return &testKey.Address, &testKey.EthAddress, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

what's in testKey.EthAddress if it's not secp256k1eth?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh it's all zeroed out

Copy link
Contributor

Choose a reason for hiding this comment

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

should we just make the test key struct have a *ethCommon.Address instead then?

@pro-wh
Copy link
Contributor

pro-wh commented Apr 27, 2023

what's the codepath that leads to this in the cli though?

@matevz matevz force-pushed the matevz/fix/client-go-test-wallet-eth branch from ab10199 to 44915f4 Compare April 27, 2023 18:58
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

thanks

@matevz
Copy link
Member Author

matevz commented Apr 27, 2023

I cleaned this and use *ethCommon.Address instead of zeros (actually leftovers from File wallet object before the cli/sdk split).

@matevz matevz changed the title client-sdk: Fix ETH address handling [BREAKING] client-sdk: Fix ETH address format for test accounts Apr 27, 2023
@matevz matevz requested a review from pro-wh May 4, 2023 12:42
@matevz matevz force-pushed the matevz/fix/client-go-test-wallet-eth branch from 44915f4 to 8218476 Compare May 4, 2023 12:42
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

oh thanks

@matevz matevz merged commit 1c910a7 into main May 8, 2023
27 checks passed
@matevz matevz deleted the matevz/fix/client-go-test-wallet-eth branch May 8, 2023 10:27
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.

Wrong warning for consensus transfer txes
3 participants