maxonrow / maxonrow-go

Maxonrow Blockchain
https://www.maxonrow.com/
5 stars 6 forks source link

multiSig Fund-transfer testing - add new test-cases #85

Closed githubckgoh1439 closed 4 years ago

githubckgoh1439 commented 4 years ago
  1. add new test-cases - for MultiSig Fund Transfer.
  2. update codec under MultiSig,
githubckgoh1439 commented 4 years ago

Hi, Mostafa

Need your comments base on these kind of scenarios:

For those cases base on the 'Delete MultiSig Tx' with those flags which as I expected, but with Error when I run. Please help to take look on it.

githubckgoh1439 commented 4 years ago

Hi, YK

Please comment on this.

We may need to move this checking from '/x/auth/handler.go' to 'app/validation.go' ==> eg. [Delete MultiSig Tx - Error, due to 'Pending tx is not found' which ID :]

where

Filename : /x/auth/handler.go [

if pendingTx == nil { return sdkTypes.ErrUnknownRequest("Pending tx is not found.").Result() }

]

yenkhoon commented 4 years ago

only one checking? how bout the others?

githubckgoh1439 commented 4 years ago

Hi, YK

This I add-in :

cdc.RegisterConcrete(MsgDeleteMultiSigTx{}, "mxw/msgDeleteMultiSigTx", nil)

Regards,

Goh

On Wed, Mar 18, 2020 at 3:10 PM yk notifications@github.com wrote:

@yenkhoon commented on this pull request.

In x/auth/codec.go https://github.com/maxonrow/maxonrow-go/pull/85#discussion_r394135612:

@@ -14,6 +14,7 @@ func RegisterCodec(cdc *codec.Codec) { cdc.RegisterConcrete(MsgCreateMultiSigTx{}, "mxw/msgCreateMultiSigTx", nil)

cdc.RegisterConcrete(MsgSignMultiSigTx{}, "mxw/msgSignMultiSigTx", nil)

  • cdc.RegisterConcrete(MsgDeleteMultiSigTx{}, "mxw/msgDeleteMultiSigTx", nil)

what is this change? i forgot to add codec?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxonrow/maxonrow-go/pull/85#pullrequestreview-376600507, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIQPD6BYANY4KUJGTIKGFVLRIBXWJANCNFSM4LOH2PUQ .

githubckgoh1439 commented 4 years ago

Hi, YK

For time-being only that, one issue I already comment in the pull request, base on the 'Delete MultiSig Tx' as I expected, but run with Error at the end. Need Boss mostafa to comment and reply.

Regards, Goh

On Wed, Mar 18, 2020 at 2:50 PM yk notifications@github.com wrote:

only one checking? how bout the others?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxonrow/maxonrow-go/pull/85#issuecomment-600453839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIQPD6H35XRDRGJZIJVTVLTRIBVL5ANCNFSM4LOH2PUQ .

yenkhoon commented 4 years ago

the rest is a very good job.

b00f commented 4 years ago

1- Please don't use Tab for indenting. This will make your code be different in other computers. You can force your vscode to always use space instead of tab. Open preferences and search for tab, the enable Insert Space feature.

2- Why some tests are commented? like case-1.1-Delete MultiSig Tx

3- Transfer ownership to non-kyc owenr. Should it work?

githubckgoh1439 commented 4 years ago

1- Please don't use Tab for indenting. This will make your code be different in other computers. You can force your vscode to always use space instead of tab. Open preferences and search for tab, the enable Insert Space feature.

Goh : OK will use this common-practice.

2- Why some tests are commented? like case-1.1-Delete MultiSig Tx

Goh : due to getting this Error Message of 'Unable to find internal transaction: '. Need your comments on this. After signed, still allow to delete the Internal-Tx ?

3- Transfer ownership to non-kyc owenr. Should it work?

Goh : Not in the current test-cases. Will add-in this. It should throw Error Message instead of proceed.

Regards,

Goh

On Wed, Mar 18, 2020 at 6:10 PM Mostafa Sedaghat Joo < notifications@github.com> wrote:

1- Please don't use Tab for indenting. This will make your code be different in other computers. You can force your vscode to always use space instead of tab. Open preferences and search for tab, the enable Insert Space feature.

2- Why some tests are commented? like case-1.1-Delete MultiSig Tx

3- Transfer ownership to non-kyc owenr. Should it work?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxonrow/maxonrow-go/pull/85#issuecomment-600533927, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIQPD6AEDZAL3NCAAXUTMMLRICMZ7ANCNFSM4LOH2PUQ .

b00f commented 4 years ago

I had updated those 'Delete MultiSig Tx' in the latest commit today, but still have problems in Case-1.1, Case-2.

Please give me the line number, So many Case-1.1 we have.

b00f commented 4 years ago

'Unable to find internal transaction'

This error means you are ging to delete an internal transaction which 1) not exist, or 2) it gt broadcast before, or) TX_ID is not correct.