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

Increase AVAX support and add AVAXC #340

Closed
wants to merge 7 commits into from
Closed

Increase AVAX support and add AVAXC #340

wants to merge 7 commits into from

Conversation

0xcharchar
Copy link
Contributor

Issue number

Missing X- and P- identity stripping as highlighted in #294

Description

Two changes are included:

  • increase compatibility with AVAX X & P chains by stripping any address that has the X- or P- identifier prepended
  • add support for AVAXC, which is the Avalanche C-Chain, including C- identifier stripping

Reference to the specification

Reference to the test address.

Both addresses were pulled from transactions on the Avalanche blockchain explorer: https://explorer.avax.network/

List of features added/changed

  • changed AVAX to support address format with and without ID prepended, previously did not support the ID
  • added AVAXC support, which is uses the EVM format and could have an ID prepended

How much has the filesize increased?

Before: 410838
After: 411043
Difference: 205 bytes

How Has This Been Tested?

Added to the existing unit test vectors for AVAX and new vectors for AVAXC

Checklist:

  • My code follows the code style of this project.
  • My code implements all the required features.
  • I have specified correct coinTypes specified at Slip 44
  • I have provided the reference link to the specification I implemented.
  • I have provided enough explanation about how I provided the test address

AVAX uses bech32 for its X & P chains. A valid address format includes
the 'X-' or 'P-' ID prepended to the address. This adds a custom decoder
to strip the ID and decode the leftover address with bech32.

Change is backwards compatible with existing functionality.
The Avalanche C-Chain (AVAXC) uses the EVM addressing system. That being
said, on Avalanche, it is valid to have the chain ID prepended to the
address. This decoder does some preparsing to strip the ID and pass the
leftover address to a decoder.
@0xcharchar
Copy link
Contributor Author

Tried to keep the changes very tight in each commit, if the team wants any of them cut out let me know

@makoto
Copy link
Member

makoto commented Jun 1, 2022

Thank you for your contribution. Any reason why package-lock.json is updated without any change on package.json?

@0xcharchar
Copy link
Contributor Author

Thank you for your contribution. Any reason why package-lock.json is updated without any change on package.json?

Packages automatically updated when I did the package install. Happy to revert that commit.

@makoto
Copy link
Member

makoto commented Jun 7, 2022

Packages automatically updated when I did the package install. Happy to revert that commit.

yes, that would be great

@0xcharchar
Copy link
Contributor Author

So, I've reverted the package updates and was getting an error in the Travis tests. Tried merging in master with no success. Eventually I cloned the branch that #341 was based on and ran the tests locally with the same error result we are seeing here in Travis.

My local result:

  ● MRX

    TypeError: Cannot read property 'decoder' of undefined

      1259 |
      1260 |     for (var example of vector.passingVectors) {
    > 1261 |       const decoded = format.decoder(example.text);
           |                              ^
      1262 |       expect(decoded).toBeInstanceOf(Buffer);
      1263 |       expect(decoded.toString('hex')).toBe(example.hex);
      1264 |       const reencoded = format.encoder(decoded);

      at Object.<anonymous> (src/__tests__/index.test.ts:1261:30)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 136 passed, 137 total
Snapshots:   0 total
Time:        9.258 s
Ran all test suites.
npm ERR! Test failed.  See above for more details.
➜  seqsee-address-encoder (master)✭ nvm current
v10.24.1
➜  seqsee-address-encoder (master)✭ git remote --v
origin  https://github.com/SeqSEE/address-encoder.git (fetch)
origin  https://github.com/SeqSEE/address-encoder.git (push)

And the direct link to Travis result for this PR

It looks like the Travis tests may not have been run for #341?

@SeqSEE @makoto How would you like to proceed on getting this sorted out?

@@ -1550,7 +1574,8 @@ export const formats: IFormat[] = [
hexChecksumChain('GO_LEGACY', 6060),
bech32mChain('XCH', 8444, 'xch', 90),
getConfig('NULS', 8964, nulsAddressEncoder, nulsAddressDecoder),
bech32Chain('AVAX', 9000, 'avax'),
getConfig('AVAX', 9000, makeBech32Encoder('avax'), makeAvaxDecoder('avax')),
Copy link
Member

@makoto makoto Jun 15, 2022

Choose a reason for hiding this comment

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

Can you verify only X and P prefixes are allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the documentation (https://support.avax.network/en/articles/4596397-what-is-an-address), X or P are allowed and are technically separate chains using the same addressing format (besides the prefix).

There seems to be a new feature, the chainId can be used instead of an alias:

Same address, using the chainID instead of the chain alias:
11111111111111111111111111111111LpoYY-avax1kj06lhgx84h39snsljcey3tpc046ze68mek3g5

Is this something you would like to see supported too?

Copy link
Member

Choose a reason for hiding this comment

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

Well spotted about chainId. Is it something we can verify the format or it can be anything?

@@ -1550,7 +1574,8 @@ export const formats: IFormat[] = [
hexChecksumChain('GO_LEGACY', 6060),
bech32mChain('XCH', 8444, 'xch', 90),
getConfig('NULS', 8964, nulsAddressEncoder, nulsAddressDecoder),
bech32Chain('AVAX', 9000, 'avax'),
getConfig('AVAX', 9000, makeBech32Encoder('avax'), makeAvaxDecoder('avax')),
getConfig('AVAXC', 9005, makeChecksummedHexEncoder(), makeAvaxcDecoder()),
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 verify only C-prefix is allowed?
Also, please use ChainID for EVM compatible chains instead of SLIP44 cointype? Please see https://docs.ens.domains/ens-improvement-proposals/ensip-11-evmchain-address-resolution for more detail. You should be able to make use of convertEVMChainIdToCoinType(chainId) which I explained at https://medium.com/the-ethereum-name-service/improved-ens-support-for-evm-compatible-chains-19fc86e9722b (but probably have to strip C- off/back in when storing)

@makoto
Copy link
Member

makoto commented Jun 15, 2022

I didn't notice that MXR PR didn't run CI test. I will revert at #343

@makoto
Copy link
Member

makoto commented Jun 16, 2022

My comment is a bit scattered so here is the summary

  • For AVAX, your current base implementation is good. Please add a check to only accept P- and X-. Let's not handle chainId for now as we don't know what format exists.
  • For AVAXC, please EVM chainID, not SLIP44 cointype as mentioned in the comment. This allows to have all EVM coins in the same ID range.

Please let me know if you have any questions.

@makoto makoto mentioned this pull request Aug 10, 2022
@makoto
Copy link
Member

makoto commented Aug 10, 2022

I split this PR into the following two PRs
#359
#358

@makoto makoto closed this Aug 10, 2022
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.

2 participants