Skip to content

CT-256 - algorand offline signing#414

Merged
argjv merged 2 commits into
masterfrom
CT-256-algorand-offline-signing
Jul 10, 2019
Merged

CT-256 - algorand offline signing#414
argjv merged 2 commits into
masterfrom
CT-256-algorand-offline-signing

Conversation

@twtaylorbitgo

@twtaylorbitgo twtaylorbitgo commented Jul 7, 2019

Copy link
Copy Markdown
Contributor

Algorand offline signing.

Note this changes the Algorand signing flow (instead of the build return json it returns the pre-signed tx encoded as base64) and is tied to a client and platform PR as well.

@codecov

codecov Bot commented Jul 7, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #414       +/-   ##
===========================================
+ Coverage    38.6%   54.88%   +16.28%     
===========================================
  Files          74       75        +1     
  Lines        9891     9915       +24     
  Branches     2157     2157               
===========================================
+ Hits         3818     5442     +1624     
+ Misses       5419     3616     -1803     
- Partials      654      857      +203
Flag Coverage Δ
#integration ?
#unit 54.88% <ø> (?)
Impacted Files Coverage Δ
src/v2/coins/token.ts 32.45% <0%> (-36.43%) ⬇️
src/v2/webhooks.ts 34.28% <0%> (-34.29%) ⬇️
src/v2/markets.ts 20% <0%> (-26.67%) ⬇️
src/v2/wallets.ts 45.35% <0%> (-20.45%) ⬇️
src/v2/pendingApproval.ts 17.51% <0%> (-19.71%) ⬇️
src/common.ts 80.64% <0%> (-9.68%) ⬇️
src/bitgo.ts 50.59% <0%> (-5.6%) ⬇️
src/wallets.ts 13.23% <0%> (-3.44%) ⬇️
src/util.ts 60.56% <0%> (-1.41%) ⬇️
src/v2/coinFactory.ts 91.75% <0%> (-1.04%) ⬇️
... and 49 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 9aedfc8...cfc97a6. Read the comment docs.

argjv
argjv suggested changes Jul 7, 2019
View reviewed changes
Comment thread modules/core/src/v2/coins/algo.ts Outdated
Comment thread modules/core/src/v2/coins/algo.ts Outdated

@argjv argjv Jul 7, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we make this optional in TransactionFee and omit it? Or is expected somewhere else?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

I think so

Comment thread modules/core/src/v2/coins/algo.ts Outdated
Comment thread modules/core/src/v2/coins/algo.ts Outdated
Comment thread modules/core/src/v2/coins/algo.ts Outdated
Comment thread modules/core/src/v2/wallet.ts Outdated
Comment thread modules/core/src/v2/wallet.ts Outdated
tylerlevine
tylerlevine suggested changes Jul 8, 2019
View reviewed changes
Comment thread modules/core/src/v2/coins/algo.ts Outdated
Comment thread modules/core/src/v2/coins/algo.ts Outdated

@tylerlevine tylerlevine Jul 8, 2019

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.

why are we returning the same data twice with two different names? Can we clean this up?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

I've removed this, yeah we didn't need to do this

Comment thread modules/core/src/v2/wallet.ts Outdated

@tylerlevine tylerlevine Jul 8, 2019

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.

Missing space before return type. Also the callback param is incorrect (type argument should always be the same as the returned promise type argument).

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

ah good point

Comment thread modules/core/src/v2/wallet.ts Outdated

@tylerlevine tylerlevine Jul 8, 2019

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 this is getting pulled out, it should get pulled out into keychains.ts, not wallets.ts. Add something like Keychains.getSigningKeychains(wallet).

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

sure but are there static methods in Keychains?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

I don't see anything that's not just attached to the prototype

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

@tylerlevine tylerlevine Jul 8, 2019

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.

is this importable? Or we need to use require here?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

hmm maybe

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

@tylerlevine tylerlevine Jul 8, 2019

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.

why not exported?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

ack

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

@tylerlevine tylerlevine Jul 8, 2019

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.

Why not exported?

@twtaylorbitgo twtaylorbitgo Jul 9, 2019

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.

ack

@tylerlevine

tylerlevine commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

Please have a look at the tests as well

@twtaylorbitgo twtaylorbitgo force-pushed the CT-256-algorand-offline-signing branch from 68ea4d4 to 8535f21 Compare July 9, 2019 18:43
argjv
argjv previously approved these changes Jul 9, 2019
View reviewed changes
@twtaylorbitgo twtaylorbitgo force-pushed the CT-256-algorand-offline-signing branch from 8535f21 to 07e95ce Compare July 9, 2019 19:53
@twtaylorbitgo twtaylorbitgo force-pushed the CT-256-algorand-offline-signing branch 2 times, most recently from 6b6721f to 6058f2a Compare July 10, 2019 03:04
@twtaylorbitgo twtaylorbitgo force-pushed the CT-256-algorand-offline-signing branch from 6058f2a to cfc97a6 Compare July 10, 2019 03:27
@argjv argjv self-requested a review July 10, 2019 03:50
@argjv argjv merged commit bc37816 into master Jul 10, 2019
@snyk-bot snyk-bot mentioned this pull request Sep 23, 2020
Closed
@0xabrarhussain 0xabrarhussain deleted the CT-256-algorand-offline-signing branch October 14, 2021 00:19
@Matthelonianxl Matthelonianxl mentioned this pull request Oct 1, 2023
Open
This was referenced Jul 10, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dannydeezy dannydeezy Awaiting requested review from dannydeezy
2 more reviewers
@argjv argjv argjv approved these changes
@tylerlevine tylerlevine tylerlevine approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@twtaylorbitgo @tylerlevine @argjv