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

Revise generate script and upgrade abigen #3

Merged
merged 14 commits into from
Jun 17, 2022

Conversation

cryptphil
Copy link
Member

This PR updates the generate.sh script to use the latest abigen version (v1.10.15) and includes the changes regarding the different bindings creations.

bindings/generate.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

Please see comment above regarding the import statement in generate.sh. I think there is an error in the package name and it would be good if we could test the script in the CI.

I see that you didn't upgrade to the latest version of abigen. What's the reason for that?

Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
@cryptphil cryptphil requested a review from matthiasgeihs June 14, 2022 12:37
@matthiasgeihs matthiasgeihs changed the title Update to abigen v1.10.15 Revise generate script and upgrade abigen Jun 14, 2022
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

You didn't want to run generate.sh in the CI to test that it is valid, as I suggested in my previous comment?

Also, we talked about changing the folder structure and removing --allow-paths. Is there a particular reason you didn't apply that? I think it is confusing and unnecessary that the solidity contracts, which are only used to generate the binding, are located at the root directory of the repository, and not with the generate script in the bindings folder. (which by the way could be renamed to binding)

bindings/generate.sh Outdated Show resolved Hide resolved
bindings/generate.sh Outdated Show resolved Hide resolved
bindings/generate.sh Outdated Show resolved Hide resolved
@matthiasgeihs
Copy link
Contributor

I noticed that locally after running the generate script git shows me a bunch of untracked files.

Please create a .gitignore file with the following entries:

/bindings/*/*.abi
/bindings/*/*.bin
/bindings/*/*.bin-runtime

@cryptphil cryptphil force-pushed the update-abigen branch 3 times, most recently from aab6b1f to c97d639 Compare June 14, 2022 16:44
@cryptphil
Copy link
Member Author

You didn't want to run generate.sh in the CI to test that it is valid, as I suggested in my previous comment?
Also, we talked about changing the folder structure and removing --allow-paths. Is there a particular reason you didn't apply that? I think it is confusing and unnecessary that the solidity contracts, which are only used to generate the binding, are located at the root directory of the repository, and not with the generate script in the bindings folder. (which by the way could be renamed to binding)

True, I forgot about this, I now changed that. @matthiasgeihs

@cryptphil cryptphil force-pushed the update-abigen branch 10 times, most recently from bc4348e to 0891c22 Compare June 15, 2022 08:10
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
@cryptphil cryptphil force-pushed the update-abigen branch 2 times, most recently from c2c667c to 47b5124 Compare June 15, 2022 13:44
@cryptphil cryptphil force-pushed the update-abigen branch 2 times, most recently from 9d3adde to 59e46c4 Compare June 15, 2022 13:48
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
@cryptphil cryptphil force-pushed the update-abigen branch 2 times, most recently from 304e33d to fc41898 Compare June 16, 2022 13:12
Signed-off-by: Philipp-Florens Lehwalder <[email protected]>
@matthiasgeihs
Copy link
Contributor

The access rights issue with the files coming out of Docker is related to: actions/runner#1317 (comment) .

Signed-off-by: Matthias Geihs <[email protected]>
matthiasgeihs
matthiasgeihs previously approved these changes Jun 17, 2022
@matthiasgeihs matthiasgeihs force-pushed the update-abigen branch 2 times, most recently from de1eb54 to 711ca64 Compare June 17, 2022 09:50
Signed-off-by: Matthias Geihs <[email protected]>
@matthiasgeihs matthiasgeihs merged commit 8fe1146 into hyperledger-labs:main Jun 17, 2022
@matthiasgeihs matthiasgeihs deleted the update-abigen branch June 17, 2022 09:56
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.

2 participants