Skip to content

Add EOS address methods to SDK#395

Merged
CapnMigraine merged 1 commit into
masterfrom
CT-479.sdk-eos-addresses
Jun 26, 2019
Merged

Add EOS address methods to SDK#395
CapnMigraine merged 1 commit into
masterfrom
CT-479.sdk-eos-addresses

Conversation

@CapnMigraine

Copy link
Copy Markdown
Contributor

Add EOS class with methods for address handling required by the client

@codecov

codecov Bot commented Jun 25, 2019

Copy link
Copy Markdown

Codecov Report

Merging #395 into master will increase coverage by 22.39%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #395       +/-   ##
===========================================
+ Coverage    38.4%   60.79%   +22.39%     
===========================================
  Files          71       74        +3     
  Lines        9682     9812      +130     
  Branches     2121     2139       +18     
===========================================
+ Hits         3718     5965     +2247     
+ Misses       5327     2913     -2414     
- Partials      637      934      +297
Flag Coverage Δ
#integration 38.45% <ø> (+0.05%) ⬆️
#unit 54.69% <ø> (?)
Impacted Files Coverage Δ
src/v2/coins/eos.ts 84.37% <0%> (ø)
src/v2/coins/teos.ts 69.56% <0%> (ø)
src/v2/trading/trade.ts 100% <0%> (ø)
src/v2/coins/bsv.ts 35.06% <0%> (+1.29%) ⬆️
src/keychains.ts 34% <0%> (+1.33%) ⬆️
src/util.ts 64.78% <0%> (+1.4%) ⬆️
src/v2/coinFactory.ts 94.84% <0%> (+2.37%) ⬆️
src/v2/coins/token.ts 71.52% <0%> (+2.64%) ⬆️
src/v2/coins/tbch.ts 78.57% <0%> (+3.57%) ⬆️
src/v2/coins/txlm.ts 78.57% <0%> (+3.57%) ⬆️
... and 44 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 d552610...682d3f4. Read the comment docs.

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks pretty good - quite similar to xlm! Just a few nits to clean up

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you need the = function part there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you need the = function part there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should throw an InvalidAddressError since we already have that error type defined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think these Errors can be InvalidAddressErrors also

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

returns void

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment thread modules/core/test/v2/unit/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this testing the same thing as the test above, except we do the crypto.randomBytes() before we call the function? If the intention is to make sure we deterministically generate a keypair from a seed, then it might make more sense to use a static seed instead of a random one and make sure you get the key you're expecting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, it makes more sense. Done.

@marktoda marktoda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah nice! Thanks for doing this. I think we copied a bit of work with CT-393-eos-sdk branch but all good, ill just merge that into this one as this is more fleshed out. Everything looks good to me, will let tyler take over

@CapnMigraine CapnMigraine force-pushed the CT-479.sdk-eos-addresses branch from b7b5e9f to d65f5d1 Compare June 25, 2019 23:28
@CapnMigraine

Copy link
Copy Markdown
Contributor Author

Ah nice! Thanks for doing this. I think we copied a bit of work with CT-393-eos-sdk branch but all good, ill just merge that into this one as this is more fleshed out. Everything looks good to me, will let tyler take over

Yes, I used your branch as a base. I only left out the logic for signing, actually.
Might be faster to create a new branch when this one is merged and add that code than to fix merge conflicts.

@CapnMigraine CapnMigraine requested a review from tylerlevine June 25, 2019 23:41

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more thing, rest looks good

Comment thread modules/core/src/v2/coins/eos.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if the spec is less than 12, then this condition is wrong and should be >=. What should the behavior be if the address is exactly 12 characters?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's up to 12 characters. Fixed the comment

@CapnMigraine CapnMigraine force-pushed the CT-479.sdk-eos-addresses branch from d65f5d1 to 1e7ccf4 Compare June 26, 2019 00:59
@CapnMigraine CapnMigraine requested a review from tylerlevine June 26, 2019 01:01
tylerlevine
tylerlevine previously approved these changes Jun 26, 2019

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@CapnMigraine CapnMigraine force-pushed the CT-479.sdk-eos-addresses branch from cccdffc to 682d3f4 Compare June 26, 2019 02:26
@CapnMigraine CapnMigraine merged commit 164693c into master Jun 26, 2019
@0xabrarhussain 0xabrarhussain deleted the CT-479.sdk-eos-addresses branch October 14, 2021 00:19
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