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

[account] Improve account experience #125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gregnazario
Copy link
Contributor

@gregnazario gregnazario commented Feb 8, 2025

Description

  1. Change authkey input to accounts to address
  2. Allow for extracting private key from accounts

Test Plan

Added 100% possible coverage on types/ folder

Related Links

1. Change authkey input to accounts to address
2. Allow for extracting private key from accounts
@gregnazario gregnazario force-pushed the improve-account-behavior branch from 257e6c3 to 60b12a7 Compare February 11, 2025 18:27
@@ -60,6 +60,42 @@ func NewSecp256k1Account() (*Account, error) {
return NewAccountFromSigner(signer)
}

// ExtractMessageSigner extracts the message signer from the account for

Choose a reason for hiding this comment

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

is for needed?

assert.NoError(t, err)
expectedEd25519String, err := ed25519PrivateKey.ToAIP80()
assert.NoError(t, err)
assert.Equal(t, expectedEd25519String, ed25519KeyString)

Choose a reason for hiding this comment

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

Maybe add another test case to verify AIP 80

assert.True(t, strings.HasPrefix(ed25519KeyString, "ed25519-priv-"))

@@ -60,6 +60,42 @@ func NewSecp256k1Account() (*Account, error) {
return NewAccountFromSigner(signer)
}

// ExtractMessageSigner extracts the message signer from the account for
func (account *Account) ExtractMessageSigner() (crypto.MessageSigner, bool) {

Choose a reason for hiding this comment

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

Is it worth returning error here as opposed to bool for consistency?

Choose a reason for hiding this comment

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

On the same consistency topic, do we need the Extract prefix in these new funcs? Other places in the sdk we use noun phrases for these derived values/attributes

}

// ExtractPrivateKeyString extracts the private key string
func (account *Account) ExtractPrivateKeyString() (string, error) {

Choose a reason for hiding this comment

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

Is it worth exposing the PrivateKey as a byte slice of the raw bytes instead of string? Alternatively, update the doc to indicate that the func returns a AIP-80 compliant string

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.

3 participants