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: Add MayaChain (CACAO) #662

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

BitHighlander
Copy link
Contributor

Screen Shot 2024-01-30 at 10 59 39 AM

Overview

This pull request introduces support for MayaChain in the HDWallet package. Additionally, it re-enables Osmosis support, which was previously removed.

Changes

  • MayaChain Integration

    • Added MayaChain as a supported chain in the HDWallet.
    • Implemented the necessary types and functions for MayaChain compatibility.
    • Included comprehensive tests for MayaChain functionalities.
  • Re-enable Osmosis

    • Restored Osmosis support

Rationale

  • MayaChain Integration
    • The addition of MayaChain is in line with our goal to expand the HDWallet's multi-chain capabilities.
    • It addresses the growing demand in the community for MayaChain support.

Testing

  • tests have been added and passed for both MayaChain and Osmosis.
  • All existing tests in the HDWallet are unaffected and continue to pass.
  • Manual testing has been conducted to ensure compatibility and stability.

Notes

  • While I understand the previous decision to remove Osmosis, I strongly believe in constructive feedback and proactive problem-solving. This PR aims to demonstrate that with a collaborative and determined approach, we can overcome challenges and enhance our offerings.

Conclusion

This PR is a significant step towards making the HDWallet more versatile and robust. I look forward to the community's feedback/

Request for comments and review: @BitHighlander. 🤖 generated pr

@BitHighlander BitHighlander requested a review from a team as a code owner January 30, 2024 16:15
Copy link

vercel bot commented Jan 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hdwallet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 12, 2024 7:23pm

return;
}
// eslint-disable-next-line no-console
console.log(core);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

}
// eslint-disable-next-line no-console
console.log(core);
if (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

?

$mayachainNativeResults.val(result);
} else {
const label = await wallet.getLabel();
$mayachainNativeResults.val(label + " does not support THORChain");
Copy link
Contributor

Choose a reason for hiding this comment

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

?

showDisplay: true,
});
$mayachainNativeResults.val(result);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this is never going to it as the else block of if (true)

return;
}
if (true) {
// eslint-disable-next-line no-console
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure about disabling this rule here?

$mayachainNativeResults.val("No wallet?");
return;
}
if (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also what?

@@ -136,6 +137,14 @@ export function integration(suite: WalletSuite): void {
thorchainTests(() => ({ wallet, info }));
});

describe("mayachainTests", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't follow the naming convention of tests in this file. Shouldn't this be MayaChainWallet ?

@@ -136,6 +137,14 @@ export function integration(suite: WalletSuite): void {
thorchainTests(() => ({ wallet, info }));
});

describe("mayachainTests", () => {
beforeAll(async () => {
wallet = await suite.createWallet("Thorchain");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

@@ -0,0 +1,7 @@
import * as core from "@shapeshiftoss/hdwallet-core";

import { mayachainTests as tests } from "./mayachain";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you follow the naming convention of the integrations module, you won't have to alias the import

if (!wallet) return;

const out = wallet.describePath({
path: core.bip32ToAddressNList("m/44'/931'/0'/0/0"),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto path question here

@@ -40,6 +40,7 @@
"@types/inquirer": "9.0.3",
"@typescript-eslint/eslint-plugin": "^6.7.3",
"@typescript-eslint/parser": "^6.7.3",
"browserify-zlib": "^0.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this as a direct dep?

@@ -158,6 +158,8 @@ const slip44Table = Object.freeze({
Gnosis: 60,
Arbitrum: 60,
ArbitrumNova: 60,
Mayachain: 931,
Cacao: 931,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Isn't CACAO the native asset of MAYA Chain?

@@ -1,6 +1,6 @@
{
"name": "@shapeshiftoss/hdwallet-native",
"version": "1.53.3",
"version": "1.52.14",
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@@ -11,7 +11,7 @@ import * as util from "./util";

const ATOM_CHAIN = "cosmoshub-4";

const protoTxBuilder = PLazy.from(() => import("@shapeshiftoss/proto-tx-builder"));
const protoTxBuilder = PLazy.from(() => import("@keepkey/proto-tx-builder"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert. Except for hdwallet-keepkey, we don't import @keepkey namespace packages in hdwallet.

"memo": "",
"msg": [
{
"type": "mayachain/MsgDeposit",
Copy link
Contributor

Choose a reason for hiding this comment

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

using amino type for proto tx

"memo": "",
"msg": [
{
"type": "mayachain/MsgSend",
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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