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

Test helpers: update and align with other contract repos #37

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Jul 13, 2020

Updates the test helpers with changes from the other contract repos and updates code styles.

Pulls in all the non-contract changes from #35, except for the package restructuring, which will be done after.

@sohkai sohkai merged commit 5a96e27 into master Jul 13, 2020
@sohkai sohkai deleted the test-helpers-align branch July 13, 2020 08:19
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

LGTM! 💪

// Sets spec id and concatenates per call
// [ 20 bytes (address) ] + [ 4 bytes (uint32: calldata length) ] + [ calldataLength bytes (payload) ]
// Defaults spec id to 1
function encodeCallScript (actions, specId = 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this logic doesn't belong to a test utility library and we are using it on several places already. Wonder if maybe a few of these across the stack that could belong to a small utility package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but it's so small that I don't mind replicating it. At some point it may be worth taking the encode/decode utilities out from aragon.js into a separate package, e.g. @aragon/contract-toolkit.

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 this pull request may close these issues.

3 participants