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

Multisend fix #106

Merged
merged 3 commits into from
Nov 23, 2021
Merged

Multisend fix #106

merged 3 commits into from
Nov 23, 2021

Conversation

germartinez
Copy link
Member

What it solves

Resolves #101

How this PR fixes it

  • Considers that transaction arrays with length = 1 are single transactions, not Multisend.
  • Does not accept transaction arrays with length = 0
  • Tests are updated to consider these use cases

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Looking good, just one small thing

): Promise<SafeTransaction> {
if (safeTransactions instanceof Array) {
const isTxArray = Array.isArray(safeTransactions)
if (isTxArray && (safeTransactions as MetaTransactionData[]).length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this always be MetaTransactionsData[]?

Copy link
Member

Choose a reason for hiding this comment

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

As well as the other casts you make in this function?

Copy link
Member

Choose a reason for hiding this comment

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

You could also just cast it once under a new variable, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be MetaTransactionsData[] or SafeTransactionDataPartial

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make a type checking function that returns value is X?

@germartinez germartinez requested a review from iamacook November 22, 2021 15:40
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

🚀

@germartinez germartinez merged commit 6cf3be1 into development Nov 23, 2021
@germartinez germartinez deleted the multisend-fix branch November 23, 2021 10:52
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update condition when creating a transaction: single vs batch
2 participants