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 #23

Merged
merged 40 commits into from
May 24, 2021
Merged

MultiSend #23

merged 40 commits into from
May 24, 2021

Conversation

germartinez
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented May 6, 2021

CLA Assistant Lite All Contributors have signed the CLA.

Copy link
Member

@rmeissner rmeissner left a comment

Choose a reason for hiding this comment

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

I would love to see some tests for the multisend encoding else there is nothing blocking

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/configuration/contracts.ts Outdated Show resolved Hide resolved
return encoded.slice(2)
}

export const encodeMultiSendData = (txs: SafeBasicTransactionData[]): string => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have tests for this method`?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is this one (not a unit test of the method), but I will add a contract call too: https://github.com/gnosis/safe-core-sdk/pull/23/files#diff-c2f7973a9f5098cdc60417d715910ce586a7eae4a43b63cd65935b545ca99791R214

Copy link
Member Author

Choose a reason for hiding this comment

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

src/utils/transactions/SafeTransaction.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@germartinez
Copy link
Member Author

Thanks @rmeissner!

@germartinez germartinez requested a review from rmeissner May 18, 2021 09:23
.github/workflows/test.yml Outdated Show resolved Hide resolved
packages/safe-core-sdk/src/EthersSafe.ts Show resolved Hide resolved
packages/safe-core-sdk/src/EthersSafe.ts Show resolved Hide resolved
packages/safe-core-sdk/src/EthersSafe.ts Show resolved Hide resolved
)
}

get safeContract() {
Copy link
Member

Choose a reason for hiding this comment

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

i'm not an oop expert but this getter is the same as just accessing the property, why make it private?

Copy link
Member Author

@germartinez germartinez May 24, 2021

Choose a reason for hiding this comment

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

to prevent it from being modified

Copy link
Member

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields#private_instance_fields

TIL you also can't access them otherwise. Btw, in safe-apps-sdk we don't use it anymore because of safe-global/safe-apps-sdk#141

packages/safe-core-sdk/src/utils/transactions/gas.ts Outdated Show resolved Hide resolved
packages/safe-core-sdk/src/utils/transactions/utils.ts Outdated Show resolved Hide resolved
tsconfig.base.json Show resolved Hide resolved
packages/safe-core-sdk/tests/utils/setup.ts Outdated Show resolved Hide resolved
packages/safe-core-sdk/tests/threshold.test.ts Outdated Show resolved Hide resolved
@germartinez germartinez requested a review from mmv08 May 24, 2021 16:24
@germartinez germartinez merged commit af1f30d into main May 24, 2021
@germartinez germartinez deleted the multisend branch May 24, 2021 16:30
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2021
@germartinez germartinez linked an issue May 27, 2021 that may be closed by this pull request
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.

Add Multisend support
3 participants