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

feat(sdk-coin-islm): add Islamic Coin #3968

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Conversation

hitansh-madan
Copy link
Contributor

@hitansh-madan hitansh-madan commented Oct 9, 2023

Ticket: WIN-490

  • adds support for different/custom pubkey types (e.g. ethsecp256k1) rather than cosmos default secp256k1
  • adds support for Islamic coin

@hitansh-madan hitansh-madan marked this pull request as ready for review October 9, 2023 12:25
@hitansh-madan hitansh-madan requested review from a team as code owners October 9, 2023 12:25
@@ -382,8 +382,8 @@ export class CosmosCoin extends BaseCoin {
const MESSAGE = Buffer.from(txHex, 'hex');

const [signA, signB] = [
MPC.sign(MESSAGE, signCombineOne.oShare, signCombineTwo.dShare, createHash('sha256')),
MPC.sign(MESSAGE, signCombineTwo.oShare, signCombineOne.dShare, createHash('sha256')),
MPC.sign(MESSAGE, signCombineOne.oShare, signCombineTwo.dShare, this.getHashFunction()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify if OVC needs a change corresponding to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it wouldn't, since this uses keccak256. And it seems in OVC we default to keccak256 unless explicitly added here

@hitansh-madan hitansh-madan force-pushed the WIN-490-add-haqq branch 2 times, most recently from 16fddeb to 42ac53a Compare October 11, 2023 06:37
Copy link
Contributor

@Vijay-Jagannathan Vijay-Jagannathan left a comment

Choose a reason for hiding this comment

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

I think both test code and service code can be squashed into a single commit. If we do really need to split them, then we'll have the rename the test commits to be prefixed with test instead

alebusse
alebusse previously approved these changes Oct 12, 2023
Copy link
Contributor

@alebusse alebusse left a comment

Choose a reason for hiding this comment

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

Changes outside of sdk-coin-islm look good too me.

@@ -0,0 +1,6 @@
export const validDenoms = ['aISLM'];
Copy link
Contributor

Choose a reason for hiding this comment

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

There must be other denoms as well, like ISLMT as seen here -
https://testnet.ping.pub/haqq/tx/87C5D925D599D1E487523E1295566475BCDE7F94846A50B32413B4580497C6A3
Can you add all denoms.
nit: should be in lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ISLMT is just how the explorer shows it, the denom in tx json still is aISLM. The denom is expected in this format on chain so lowercase wouldn't have worked.

modules/sdk-coin-islm/src/islm.ts Show resolved Hide resolved
@hitansh-madan hitansh-madan merged commit 5350cb6 into master Oct 13, 2023
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.

6 participants