Skip to content

STLX-15822_add_explainTransaction_implemented_in_account_lib_call#2237

Merged
CapnMigraine merged 1 commit into
masterfrom
STLX-15822_add_explainTransaction_implemented_in_account_lib_call
May 3, 2022
Merged

STLX-15822_add_explainTransaction_implemented_in_account_lib_call#2237
CapnMigraine merged 1 commit into
masterfrom
STLX-15822_add_explainTransaction_implemented_in_account_lib_call

Conversation

@fmartinAltamirano

Copy link
Copy Markdown
Contributor

Add explainTransaction implemented in account lib call in core.

STLX-15822

@codecov

codecov Bot commented Apr 28, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2237 (81d0861) into master (5113869) will increase coverage by 52.61%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2237       +/-   ##
===========================================
+ Coverage        0   52.61%   +52.61%     
===========================================
  Files           0        6        +6     
  Lines           0      574      +574     
  Branches        0       94       +94     
===========================================
+ Hits            0      302      +302     
- Misses          0      262      +262     
- Partials        0       10       +10     
Flag Coverage Δ
unit 52.61% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/express/src/expressApp.ts 89.51% <0.00%> (ø)
modules/express/src/args.ts 100.00% <0.00%> (ø)
modules/express/src/retryPromise.ts 11.76% <0.00%> (ø)
modules/express/src/errors.ts 82.35% <0.00%> (ø)
modules/express/src/clientRoutes.ts 32.48% <0.00%> (ø)
modules/express/src/config.ts 94.73% <0.00%> (ø)

});
});

describe('Explain Transactions:', () => {

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.

Get Invalid cases.

Comment thread modules/bitgo/src/v2/coins/near.ts Outdated
Comment on lines +168 to +171
let rawTxBase64 = params.txPrebuild.txHex;
if (HEX_REGEX.test(params.txPrebuild.txHex)) {
rawTxBase64 = Buffer.from(params.txPrebuild.txHex, 'hex').toString('base64');
}

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.

Maybe be can do something like
const txHex = params.txPrebuild.txHex;
const rawTxBase64 = HEX_REGEX.test(txHex) ? Buffer.from(txHex, 'hex').toString('base64') : txHex;

I propose this because because Im kind of confused, params.txPrebuild.txHex is always a hex? or it could come as base64? if we always expect a hex, lets just the try catch do his job, delete the if.

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.

Well, I'm going to try this refactoring

Comment thread modules/bitgo/src/v2/coins/near.ts Outdated
Comment on lines +178 to +180
const explainedTransaction = (rebuiltTransaction as accountLib.BaseCoin.BaseTransaction).explainTransaction();

return explainedTransaction as NearTransactionExplanation;

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 we can just add the type to the statement instead of use 'as'
let rebuiltTransaction : accountLib.BaseCoin.BaseTransaction;

and then
const explainedTransaction: NearTransactionExplanation = rebuiltTransaction.explainTransaction(); return explainedTransaction;

Comment on lines +135 to +154
const explainedTransaction = await basecoin.explainTransaction({
txPrebuild: {
txHex: rawTx.transfer.signed,
},
});

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.

What happen if we add
feeInfo: {
fee: '5000',
},

to the parameters? it is allowed by the function, can we add a test with that?

Comment thread modules/bitgo/src/v2/coins/near.ts Outdated
throw new MethodNotImplementedError('Near recovery not implemented');
): Promise<NearTransactionExplanation> {
const factory = accountLib.register(this.getChain(), accountLib.Near.TransactionBuilderFactory);
let rebuiltTransaction;

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.

reorder code here

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.

If I declare rebuiltTransaction as a constant inside the try block, I cannot use it later in line 175 because it is outside the scope of the try, that is why it was defined outside.

@fmartinAltamirano fmartinAltamirano force-pushed the STLX-15822_add_explainTransaction_implemented_in_account_lib_call branch 2 times, most recently from 5802ee6 to 92e1d6b Compare May 2, 2022 16:04
@fmartinAltamirano fmartinAltamirano force-pushed the STLX-15822_add_explainTransaction_implemented_in_account_lib_call branch from 92e1d6b to 09ec016 Compare May 2, 2022 17:42
@rliberal-simtlix rliberal-simtlix marked this pull request as ready for review May 2, 2022 18:39
ewangbitgo
ewangbitgo previously approved these changes May 2, 2022
ipelaez-simtlix
ipelaez-simtlix previously approved these changes May 3, 2022

@ipelaez-simtlix ipelaez-simtlix 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 previously approved these changes May 3, 2022
Comment thread modules/bitgo/src/v2/coins/near.ts Outdated
Comment on lines 185 to 186

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.

nit: redundant variable

silalvado
silalvado previously approved these changes May 3, 2022
alebusse
alebusse previously approved these changes May 3, 2022
@fmartinAltamirano fmartinAltamirano force-pushed the STLX-15822_add_explainTransaction_implemented_in_account_lib_call branch from 09ec016 to 5aa5b50 Compare May 3, 2022 18:10
CapnMigraine
CapnMigraine previously approved these changes May 3, 2022
…n core

Add explainTransaction implemented in account lib call in core

STLX-15822
@fmartinAltamirano fmartinAltamirano force-pushed the STLX-15822_add_explainTransaction_implemented_in_account_lib_call branch from 5aa5b50 to 81d0861 Compare May 3, 2022 18:16
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.

7 participants