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 balance precheck to eth_sendRawtransaction #293

Merged
merged 3 commits into from
Jul 5, 2022
Merged

Conversation

ar-conmit
Copy link
Collaborator

@ar-conmit ar-conmit commented Jul 5, 2022

Description:

  • Add balance precheck
  • Add optional chaining to getTransactionByHash for null value of contractResult.to causing "multiple done() method invoked" error in acceptance tests
  • Refactor jsonRpcError assertion to accept predefined error object

Related issue(s):

Fixes #130

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@ar-conmit ar-conmit self-assigned this Jul 5, 2022
@Nana-EC Nana-EC added enhancement New feature or request limechain P2 labels Jul 5, 2022
@Nana-EC Nana-EC added this to the 0.3.0 milestone Jul 5, 2022
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG, minor suggestions then should be good to go

nonce: await relay.getAccountNonce(accounts[2].address)
};
const signedTx = await accounts[2].wallet.signTransaction(transaction);
await relay.callFailing('eth_sendRawTransaction', [signedTx], -32000, 'Insufficient funds for transfer');
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should pull code and error message from errors.ts itself to avoid breaking test case on changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and below tests

const balance = await this.executeQuery(new AccountBalanceQuery()
.setAccountId(account));

return ethers.BigNumber.from(balance.hbars.toTinybars().toString()).mul(10 ** 10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

set multiplier as static

@@ -275,6 +275,13 @@ export default class ServicesClient {
const receipt = await response.getReceipt(this.client);
this.logger.info(`File ${fileId} updated with status: ${receipt.status.toString()}`);
}

async getAccountBalanceInWeiBars(account: string | AccountId) {
const balance = await this.executeQuery(new AccountBalanceQuery()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pull out AccountBalanceQuery execution into a separate method as it's used multiple times in this class
Will need to update it to handle AccountId also

Suggested change
const balance = await this.executeQuery(new AccountBalanceQuery()
const balance = await this.getAccountBalance(account)

e.g.

    async getAccountBalance(account: string | AccountId): Promise<AccountBalance> {
        return this.executeQuery(new AccountBalanceQuery()
            .setAccountId(AccountId.fromString(account)), this.clientMain);
    }

@sonarcloud
Copy link

sonarcloud bot commented Jul 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ar-conmit ar-conmit requested a review from Nana-EC July 5, 2022 15:36
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG

@ar-conmit ar-conmit merged commit b1a9155 into main Jul 5, 2022
@ar-conmit ar-conmit deleted the 130-balance-precheck branch July 5, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request limechain P2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add balance precheck to eth_sendRawtransaction
2 participants