Skip to content

Update AVAX client implementation and interface to align types #301

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 14, 2023
Merged

Update AVAX client implementation and interface to align types #301

merged 1 commit into from
Aug 14, 2023

Conversation

@StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Aug 14, 2023 •
edited
Loading

Why this should be merged

Updating the GetAtomicUTXOs call on the Client is required for C-chain support in the avalanchego wallet. Additionally, this clarifies the address format (eth or avax) for addresses provided over the client.

How this works

  • Updates the client interface to take in structured types.
  • Supports non-contextual addresses to be provided on the server side (and provides non-contextual addresses on the client side)

How this was tested

  • CI
  • Using the C-chain implementation in the avalanchego wallet

@StephenButtolph StephenButtolph mentioned this pull request Aug 14, 2023
Merged
marun
marun reviewed Aug 14, 2023
View reviewed changes
plugin/evm/client.go
utxos := make([][]byte, len(res.UTXOs))
for i, utxo := range res.UTXOs {
b, err := formatting.Decode(formatting.Hex, utxo)
utxoBytes, err := formatting.Decode(res.Encoding, utxo)
Copy link

@marun marun Aug 14, 2023

Choose a reason for hiding this comment

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

(No action required) It may be worth separating non-functional (e.g. naming) from functional changes when refactoring to simplify review.

marun
marun reviewed Aug 14, 2023
View reviewed changes
plugin/evm/client.go
res := &api.JSONTxID{}
err := c.requester.SendRequest(ctx, "avax.import", &ImportArgs{
UserPass: user,
To: to,
Copy link

@marun marun Aug 14, 2023

Choose a reason for hiding this comment

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

(No action required) Why is it necessary to convert the address to hex for avax.exportKey but not here for avax.import?

Copy link
Contributor Author

@StephenButtolph StephenButtolph Aug 14, 2023

Choose a reason for hiding this comment

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

We could update the ExportKeyArgs.Address var to be common.Address rather than string and I think we'd be able to avoid calling .Hex().

marun
marun reviewed Aug 14, 2023
View reviewed changes
plugin/evm/service.go
return errors.New("argument 'amount' must be > 0")
}

// Get the chainID and parse the to address
Copy link

@marun marun Aug 14, 2023

Choose a reason for hiding this comment

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

(No action required) Would it make sense to conditionally use the alternate parsing strategy based on the returned error? ParseAddress returns 4 different errors and I'm curious if all of them are appropriate prompts to follow the other strategy.

Or maybe the use of a full address could be deprecated and eventually removed? I'm not sure why its desirable to support the provision of the chain id in both ways vs picking one or the other.

@aaronbuchwald aaronbuchwald merged commit 00214b1 into master Aug 14, 2023
@aaronbuchwald aaronbuchwald deleted the update-client-interface branch August 14, 2023 18:58
joshua-kim added a commit that referenced this pull request Aug 29, 2023
@joshua-kim
Squashed commit of the following:
9d6675c 
commit 4ef26a0
Author: Darioush Jalali <[email protected]>
Date:   Mon Aug 28 06:38:44 2023 -0700

    bump go version in dockerfile (#311)

commit 505a6ab
Author: aaronbuchwald <[email protected]>
Date:   Fri Aug 25 15:43:09 2023 -0400

    core/state/snapshot: increase batch size during diffToDisk (#309)

commit cca3e00
Author: marun <[email protected]>
Date:   Fri Aug 25 09:44:42 2023 -0700

    Bump golang version to 1.19.12 for CI (#306)

    * Bump golang version to 1.19.12 for CI

    Also bump targeted version of avalanchego to the v1.10.8 (which also
    uses golang 1.19.12).

    * fixup: Update go-version specification for consistency with other repos

commit 42ba830
Author: Darioush Jalali <[email protected]>
Date:   Fri Aug 25 09:44:12 2023 -0700

    add upstream test: TestClientBatchRequest_len (#307)

commit 08e2b6e
Author: Stephen Buttolph <[email protected]>
Date:   Wed Aug 23 18:09:51 2023 -0400

    Update avalanchego dependency to v1.10.9-rc.4 (#308)

commit 5cd24c3
Author: Darioush Jalali <[email protected]>
Date:   Wed Aug 23 08:48:01 2023 -0700

    node/config.go: remove unused options (#304)

    * node/config.go: remove unused graphql options

    * remove more unused config

commit 8ea8b18
Author: aaronbuchwald <[email protected]>
Date:   Tue Aug 15 11:18:29 2023 -0400

    Add security file based on AvalancheGo (#303)

commit 00214b1
Author: Stephen Buttolph <[email protected]>
Date:   Mon Aug 14 14:58:07 2023 -0400

    Update AVAX client implementation and interface to align types (#301)

commit 5666553
Author: Ceyhun Onur <[email protected]>
Date:   Mon Aug 14 21:38:04 2023 +0300

    renamed flags (#298)

    * renamed flags

    * rename admin api flags

    * add deprecation

commit e7645fa
Author: Ikko Eltociear Ashimine <[email protected]>
Date:   Thu Aug 10 01:21:44 2023 +0900

    Fix typo in sync/README.md (#296)
joshua-kim added a commit that referenced this pull request Aug 30, 2023
@joshua-kim
Squashed commit of the following:
7920fae 
commit 4ef26a0
Author: Darioush Jalali <[email protected]>
Date:   Mon Aug 28 06:38:44 2023 -0700

    bump go version in dockerfile (#311)

commit 505a6ab
Author: aaronbuchwald <[email protected]>
Date:   Fri Aug 25 15:43:09 2023 -0400

    core/state/snapshot: increase batch size during diffToDisk (#309)

commit cca3e00
Author: marun <[email protected]>
Date:   Fri Aug 25 09:44:42 2023 -0700

    Bump golang version to 1.19.12 for CI (#306)

    * Bump golang version to 1.19.12 for CI

    Also bump targeted version of avalanchego to the v1.10.8 (which also
    uses golang 1.19.12).

    * fixup: Update go-version specification for consistency with other repos

commit 42ba830
Author: Darioush Jalali <[email protected]>
Date:   Fri Aug 25 09:44:12 2023 -0700

    add upstream test: TestClientBatchRequest_len (#307)

commit 08e2b6e
Author: Stephen Buttolph <[email protected]>
Date:   Wed Aug 23 18:09:51 2023 -0400

    Update avalanchego dependency to v1.10.9-rc.4 (#308)

commit 5cd24c3
Author: Darioush Jalali <[email protected]>
Date:   Wed Aug 23 08:48:01 2023 -0700

    node/config.go: remove unused options (#304)

    * node/config.go: remove unused graphql options

    * remove more unused config

commit 8ea8b18
Author: aaronbuchwald <[email protected]>
Date:   Tue Aug 15 11:18:29 2023 -0400

    Add security file based on AvalancheGo (#303)

commit 00214b1
Author: Stephen Buttolph <[email protected]>
Date:   Mon Aug 14 14:58:07 2023 -0400

    Update AVAX client implementation and interface to align types (#301)

commit 5666553
Author: Ceyhun Onur <[email protected]>
Date:   Mon Aug 14 21:38:04 2023 +0300

    renamed flags (#298)

    * renamed flags

    * rename admin api flags

    * add deprecation

commit e7645fa
Author: Ikko Eltociear Ashimine <[email protected]>
Date:   Thu Aug 10 01:21:44 2023 +0900

    Fix typo in sync/README.md (#296)
joshua-kim added a commit to joshua-kim/coreth that referenced this pull request Aug 30, 2023
@joshua-kim
Squashed commit of the following:
d8aab62 
commit 3f5dc8a
Author: marun <[email protected]>
Date:   Tue Aug 29 10:17:05 2023 -0700

    e2e: Add avalanchego e2e job (ava-labs#305)

    * e2e: Add avalanchego e2e job

    * fixup: Use currently supported golang version

    * fixup: Enable configurable avalanchego clone path

commit 4ef26a0
Author: Darioush Jalali <[email protected]>
Date:   Mon Aug 28 06:38:44 2023 -0700

    bump go version in dockerfile (ava-labs#311)

commit 505a6ab
Author: aaronbuchwald <[email protected]>
Date:   Fri Aug 25 15:43:09 2023 -0400

    core/state/snapshot: increase batch size during diffToDisk (ava-labs#309)

commit cca3e00
Author: marun <[email protected]>
Date:   Fri Aug 25 09:44:42 2023 -0700

    Bump golang version to 1.19.12 for CI (ava-labs#306)

    * Bump golang version to 1.19.12 for CI

    Also bump targeted version of avalanchego to the v1.10.8 (which also
    uses golang 1.19.12).

    * fixup: Update go-version specification for consistency with other repos

commit 42ba830
Author: Darioush Jalali <[email protected]>
Date:   Fri Aug 25 09:44:12 2023 -0700

    add upstream test: TestClientBatchRequest_len (ava-labs#307)

commit 08e2b6e
Author: Stephen Buttolph <[email protected]>
Date:   Wed Aug 23 18:09:51 2023 -0400

    Update avalanchego dependency to v1.10.9-rc.4 (ava-labs#308)

commit 5cd24c3
Author: Darioush Jalali <[email protected]>
Date:   Wed Aug 23 08:48:01 2023 -0700

    node/config.go: remove unused options (ava-labs#304)

    * node/config.go: remove unused graphql options

    * remove more unused config

commit 8ea8b18
Author: aaronbuchwald <[email protected]>
Date:   Tue Aug 15 11:18:29 2023 -0400

    Add security file based on AvalancheGo (ava-labs#303)

commit 00214b1
Author: Stephen Buttolph <[email protected]>
Date:   Mon Aug 14 14:58:07 2023 -0400

    Update AVAX client implementation and interface to align types (ava-labs#301)

commit 5666553
Author: Ceyhun Onur <[email protected]>
Date:   Mon Aug 14 21:38:04 2023 +0300

    renamed flags (ava-labs#298)

    * renamed flags

    * rename admin api flags

    * add deprecation

commit e7645fa
Author: Ikko Eltociear Ashimine <[email protected]>
Date:   Thu Aug 10 01:21:44 2023 +0900

    Fix typo in sync/README.md (ava-labs#296)
@knikos knikos mentioned this pull request Feb 28, 2024
Merged
darioush pushed a commit that referenced this pull request Apr 25, 2024
@felipemadero
add more osxcross dependencies (#301)
2a06cd1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronbuchwald aaronbuchwald aaronbuchwald approved these changes

@darioush darioush Awaiting requested review from darioush

@ceyonur ceyonur Awaiting requested review from ceyonur

+1 more reviewer

@marun marun marun approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

No open projects
EVM
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@StephenButtolph @marun @aaronbuchwald