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

add code property on the BroadcastTxSuccess interface. #878

Closed
fly33499 opened this issue Sep 20, 2021 · 4 comments · Fixed by #950
Closed

add code property on the BroadcastTxSuccess interface. #878

fly33499 opened this issue Sep 20, 2021 · 4 comments · Fixed by #950
Milestone

Comments

@fly33499
Copy link

Hi. I found weird things on @cosmjs/stargate/build/stargateclient.ts.

Looking at the results obtained through the following code, "code" property is included in both success and failure.

But, BroadcastTxSucess did not have a code property, so it was difficult to check whether it was successful. I can find it in the log, but it was not on the interface, so I added one line of code as follows, and I was able to get the desired result.

let result await SigningStargateClient.signAndBroadcast(...);
console.log(result);

// on success

{
  code: 0,
  height: 219894,
  rawLog: '[{"events":[{"type":"message","attributes":[{"key":"action","value":"send"},{"key":"sender","value":"firma1trqyle9m2nvyafc2n25frkpwed2504y6avgfzr"},{"key":"module","value":"bank"}]},{"type":"transfer","attributes":[{"key":"recipient","value":"firma12er8ls2sf5zess3jgjxz59xat9xtf8hz0hk6n4"},{"key":"sender","value":"firma1trqyle9m2nvyafc2n25frkpwed2504y6avgfzr"},{"key":"amount","value":"2000000ufct"}]}]}]',
  transactionHash: 'C0B416CA868C55C2B8C1BBB8F3CFA233854F13A5CB15D3E9599F50CAF7B3D161',
  gasUsed: 61556,
  gasWanted: 200000
}

// on fail

{
code: 5,
height: 219901,
rawLog: 'failed to execute message; message index: 0: 1855527000ufct is smaller than 20000000000000000000000ufct: insufficient funds',
transactionHash: 'FDC4FB701AABD465935F7D04AE490D1EF5F2BD4B227601C4E98B57EB077D9B7D',
gasUsed: 54396,
gasWanted: 200000
}

I added one line code on the BroadcastTxSuccess.

export declare type BroadcastTxResponse = BroadcastTxSuccess | BroadcastTxFailure;

export interface BroadcastTxFailure {
    readonly height: number;
    readonly code: number;
    readonly transactionHash: string;
    readonly rawLog?: string;
    readonly data?: readonly MsgData[];
}
export interface BroadcastTxSuccess {
    readonly height: number;
    readonly code: number; // I added this property.
    readonly transactionHash: string;
    readonly rawLog?: string;
    readonly data?: readonly MsgData[];
    readonly gasUsed: number;
    readonly gasWanted: number;
}

So, I can figure out the transaction is success or not by compare code property below code.

var result = await wallet.send(targetAddress, amount);
console.log(result);

if(result.code == 0){ // success
}
else{ // error case

}

var result = await wallet.send(targetAddress, amount);
console.log(result);

// success case
expect(result.code).to.equal(0);

// error case
expect(result.code).to.equal(5);

What do you think about this?

fly33499 added a commit to FirmaChain/cosmjs that referenced this issue Sep 24, 2021
@webmaster128
Copy link
Member

Since the code is always 0 in BroadcastTxSuccess, I see little reason to add it to the interface.

Please use isBroadcastTxFailure(result) and isBroadcastTxSuccess(result) to check if a BroadcastTxResponse is sucess of failure. This gives you additional type safety via TypeScript.

For tests there is also assertIsBroadcastTxSuccess(result) that throws in case of a failure. It also does type-narrowing, i.e. after the call TY knows result is BroadcastTxSuccess.

@webmaster128
Copy link
Member

I'll add this in 0.27.0 as there is nothing wrong with the suggestion. However, using isBroadcastTxFailure/isBroadcastTxSuccess (renamed to isDeliverTxFailure/isDeliverTxSuccess) is still a better way to make the differentiation.

@webmaster128
Copy link
Member

Hmm, looking at it again, probably makes no sense to have different types here at all. DeliverTxFailure should have gasUsed and gasWanted too because those will be populated in case of an out of gas error.

@webmaster128
Copy link
Member

That case means the transaction is accepted on chain but the execution failed. The naming of the type was confusing. It was renamed to DeliverTxResponse now. Please check the more recent versions of CosmJS. This should serve your use case better.

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 a pull request may close this issue.

2 participants