-
Notifications
You must be signed in to change notification settings - Fork 283
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
Conversation
86ab885
to
990b287
Compare
@@ -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()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
16fddeb
to
42ac53a
Compare
42ac53a
to
49f8ad9
Compare
There was a problem hiding this 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
49f8ad9
to
456cb30
Compare
There was a problem hiding this 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']; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
456cb30
to
c49bdd1
Compare
Ticket: WIN-490
ethsecp256k1
) rather than cosmos defaultsecp256k1