Skip to content

Add ETH estimates #33

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

Merged
merged 7 commits into from
Jul 21, 2021
Merged

Add ETH estimates #33

merged 7 commits into from
Jul 21, 2021

Conversation

pcothenet
Copy link
Contributor

@pcothenet pcothenet commented Jul 15, 2021

What

  • Add ETH estimates

Why

  • Proof of stake >> Proof of work

SDK Release Checklist

  • Have you added an integration test for the changes?
  • Have you built the gem locally and made queries against it successfully?
  • Did you update the changelog?
  • Did you bump the package version?
  • For breaking changes, did you plan for the release of the new SDK versions and deploy the API to production?

end

# Create an ethereum estimate given a timestamp and gas used
# Creates an ethereum estimate for the amount of CO2 to be compensated. An order in the `draft` state may be created based on the parameters, linked to the estimate.
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming this is an html encoding bug with openapi but there are ` chars in this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Let me check the swagger file to see if the issue is there or is due to code generation.

@@ -99,4 +99,37 @@
expect(bitcoin_estimate.data.type).to eq 'bitcoin'
expect(bitcoin_estimate.data.mass_g).to be < bitcoin_estimate_2.data.mass_g
end

it 'supports creating bitcoin estimates with a timestamp' do
Copy link
Contributor

Choose a reason for hiding this comment

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

the sdk code is autogenerated but I'm assuming the tests are written manually. Can you please confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to wrap each group of it for a method being tested in a describe block. On a second take, I realized this test was for the bitcoin method and not the ethereum method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Code is auto-generated, tests are manual.

Good call on the grouping, I'll update this PR.

end

it 'supports creating ethereum estimates with partial information' do
ethereum_estimate = Patch::Estimate.create_ethereum_estimate({ create_order: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

the hash curly braces {} here are redundant


expect(ethereum_estimate.data.type).to eq 'ethereum'
expect(ethereum_estimate.data.mass_g).to be >= 2_000
end
Copy link
Contributor

@kleinjm kleinjm Jul 20, 2021

Choose a reason for hiding this comment

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

seems there are tests for the use of create_order and gas_used params. Should there be tests for project_id and timestamp as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory yes, let me add a few more.

Copy link
Contributor

@kleinjm kleinjm left a comment

Choose a reason for hiding this comment

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

no blockers but I have quite a bit to learn before I can give a PR like this a thorough review. I'm curious what we look for in these PRs as the code is auto generated. I'd love a rundown on rswag, openapi, and our sdks soon

@pcothenet pcothenet merged commit 8f3adf9 into main Jul 21, 2021
@pcothenet pcothenet deleted the pc/171 branch July 21, 2021 19:11
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