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

Bump blockchain and kliento deps #182

Merged
merged 6 commits into from
Jan 28, 2022
Merged

Conversation

37ng
Copy link
Contributor

@37ng 37ng commented Jan 20, 2022

  • Bump celo-blockchain to v1.5.1
    • Replace SendRawTransaction with SendTransaction
  • Use golang:1.17-alpine & celo-blockchain@v1.5.1 node in Dockerfile
  • Change go directive to 1.15 to align with celo-blockchain and go-ethereum
  • Update README.md

Due to changes in celo-blockchain@v.1.5.0::ethClient
- Bump celo-blockchain to v1.5.0
- Use celo-blockchain@v1.5.0 node in Dockerfile
- Change go directive to 1.15 to align with celo-blockchain and go-ethereum
- Update README.md
@37ng 37ng requested a review from mcortesi January 20, 2022 17:47
Copy link
Contributor

@piersy piersy left a comment

Choose a reason for hiding this comment

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

The version of kliento that is set in go.mod is not correct.

The tag although marked as upgrading to celo blockchain v1.5.0 actually is pointing at an earlier commit.

But even then we don't currently have a correct commit to point to, because the latest commit on master celo-org/kliento@80b383b contains contract bindings generated for the core contracts at a version different to core-contracts.v5

So before we can consider merging this we need to fix kliento

@piersy
Copy link
Contributor

piersy commented Jan 20, 2022

@YoduYodu I've deleted the v0.2.1 tag in kliento. So when kliento is updated with the correct contract bindings we can tag that as v0.2.1

@37ng 37ng marked this pull request as draft January 20, 2022 20:50
@37ng
Copy link
Contributor Author

37ng commented Jan 20, 2022

@YoduYodu I've deleted the v0.2.1 tag in kliento. So when kliento is updated with the correct contract bindings we can tag that as v0.2.1

Agree, just made this PR as draft.
I'll update Kliento once celo-blockchain v1.5.1 is released

@37ng 37ng marked this pull request as ready for review January 28, 2022 19:15
@piersy
Copy link
Contributor

piersy commented Jan 28, 2022

@YoduYodu the branch has conflicts

@37ng 37ng requested a review from piersy January 28, 2022 19:29
@piersy
Copy link
Contributor

piersy commented Jan 28, 2022

Hey @YoduYodu I would suggest splitting this PR into one that updates deps and one that just handles bumping the version. This is definitely not just a bugfix, functionality has been changed since the last release so I think the version should be at least 0.9.0.

@eelanagaraj I see you have made a lot of releases, just wondering what you would suggest for the version number.

@37ng 37ng changed the title Bump blockchain and kliento deps for v0.8.6 Bump blockchain and kliento deps Jan 28, 2022
@piersy
Copy link
Contributor

piersy commented Jan 28, 2022

Hey @YoduYodu

What I meant by bumping the version was the change from v0.8.5 -> v0.8.6 I think that could be done separately from updating the code and the golang and celo-blockchain version changes.

With regards to choosing the version number, I assumed that Semantic Versioning is being used because we are specifying the version with 3 dot separated numbers, otherwise just a single number would suffice.

And in the case of Semantic Versioning the minor number is changed when backwards compatible functionality is added, which I would say has been done because new command flags have been added since the last release.

Having looked at the changes, I'm actually not sure if they are backwards compatible, if they are not it should be a major version change, but then I think we would have to agree on the meaning of backwards compatible for this repo.

Copy link
Contributor

@piersy piersy 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 now!

@37ng 37ng merged commit 1f29fad into master Jan 28, 2022
@37ng 37ng deleted the tong/support-celo-blockchain-1.5.0 branch January 28, 2022 21:14
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.

None yet

2 participants