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

Feature/crosschain-transactions #10

Open
wants to merge 96 commits into
base: devop/release-1-39
Choose a base branch
from

Conversation

Takadenoshi
Copy link

No description provided.

andborges and others added 30 commits May 7, 2024 09:29
…ransaction

Initial crosschain transaction implementation
…plement-activity-list-user-feedback-for-crosschain
…ferent-chain

Bug/send same account different chain; Some other small UI fixes
…plement-activity-list-user-feedback-for-crosschain
…activity-list-user-feedback-for-crosschain

Identifying in the activities when a transaction is Crosschain
… into feature/kad-13-implement-activity-list-user-feedback-for-crosschain
KevinKons and others added 20 commits June 3, 2024 17:21
… into feature/kad-31-fix-wording-when-its-sendingreceiving-and-fix-sign-check-if
…ommunity/enKrypt into feature/crosschain-transactions
… into feature/kad-31-fix-wording-when-its-sendingreceiving-and-fix-sign-check-if
…ng-when-its-sendingreceiving-and-fix-sign-check-if

Feature/kad 31 fix wording when its sendingreceiving and fix sign check if
…rdware wallet message from crosschain transaction verification
…ommunity/enKrypt into feature/crosschain-transactions
@KevinKons KevinKons marked this pull request as ready for review June 5, 2024 14:07
@KevinKons KevinKons changed the title Feature/crosschain transactions DRAFT Feature/crosschain transactions Jun 5, 2024
@KevinKons KevinKons changed the title Feature/crosschain transactions Feature/crosschain-transactions Jun 5, 2024
}
if (!toAccount && activity.crossChainAccount) {
toAccount = activity.crossChainAccount;
const groupActivities = activities
Copy link

Choose a reason for hiding this comment

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

this can be simplified:

const groupdActivities = activities.reduce((acc: any, activity: any) => {
  if (activity.chain === chainId || activity.crossChainId === chainId) {
    if (!acc[activity.requestKey] || activity.idx !== 0) {
      acc[activity.requestKey] = activity;
    }
  }
  return acc;
}, {});

@@ -0,0 +1,327 @@
// Copied from https://github.com/obsidiansystems/hw-app-kda/blob/develop/src/Kadena.ts
Copy link

Choose a reason for hiding this comment

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

should be build from kadena client

Copy link

@barthuijgen barthuijgen Jul 5, 2024

Choose a reason for hiding this comment

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

I see this file was copied over, but I would strongly reconsider this approach, if you copy code you also take on the burden of maintaining it, and this code is not maintainable. some notable issues across this file:

  • No triple equals conditions used
  • blake2b dependency is not in package.json
  • Building a transaction with string concatenation is not acceptable
  • Derivation path is incorrect: should be m/44'/626'/0' instead of m/44'/626'/0'/0/0
  • Debug console.log's left in

@barthuijgen
Copy link

There are eslint config files across the different packages, but a lot of them have significant amounts of errors and don't seem to be tested at all. yarn lint only applies prettier, not eslint. As an example running npx eslint . in packages/hw-wallets gives 80 errors.

@barthuijgen
Copy link

I tried testing my ledger device on this branch, but it could not connect.

Initially giving the error Ledger device: UNKNOWN_ERROR (0x6e01) and when pressing retry it gives the error This device is already open.

Because of this I could not test any of the ledger integration.


const gasLimit = transactionResult.metaData?.publicMeta?.gasLimit;
const gasPrice = transactionResult.metaData?.publicMeta?.gasPrice;
const gasFee = gasLimit && gasPrice ? gasLimit * gasPrice : 0;

Choose a reason for hiding this comment

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

I don't understand this gasFee calculation. gasLimit should not be part of it, that's just the upper limit of what can be spend on a transaction and has nothing to do with the actual fee.

const gasLimit = transactionResult.metaData?.publicMeta?.gasLimit;
const gasPrice = transactionResult.metaData?.publicMeta?.gasPrice;
const gasFee = gasLimit && gasPrice ? gasLimit * gasPrice : 0;
gasFee = gasLimit && gasPrice ? gasLimit * gasPrice : 0;

Choose a reason for hiding this comment

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

Same comment as before, this is not a good calculation of gasFee.


const gasLimit = transactionResult.metaData?.publicMeta?.gasLimit;
const gasPrice = transactionResult.metaData?.publicMeta?.gasPrice;
const gasFee = gasLimit && gasPrice ? gasLimit * gasPrice : 0;

Choose a reason for hiding this comment

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

Same comment as before, this is not a good calculation of gasFee.

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.

5 participants