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

Implement seal_rent_params #755

Merged
merged 13 commits into from
May 26, 2021
Merged

Implement seal_rent_params #755

merged 13 commits into from
May 26, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Apr 1, 2021

Needed for #539.

We haven't yet finalized this API as stable, hence I've put it behind a feature flag for now.

Examples will follow in a separate PR.

@cmichi cmichi requested a review from athei April 1, 2021 03:14
@Robbepop
Copy link
Collaborator

Robbepop commented Apr 1, 2021

I have had a discussion with @athei where he told me that changes to the API are already in the pipeline about a less expressive but still "good enough" API using a single seal_rent_deposit API to query the rent deposit with the current storage used for the contract. Note that this API would be called multiple times during a call. However, the good part is that this would not require manually calculating storage fees on the contract side.

@athei
Copy link
Contributor

athei commented Apr 13, 2021

Yes. I talked to @cmichi about that. I will look into the next iteration of the seal_rent_deposit API soon.

@cmichi cmichi marked this pull request as draft April 16, 2021 04:41
@cmichi cmichi marked this pull request as ready for review May 19, 2021 07:15
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021

Codecov Report

Merging #755 (dec4434) into master (5571724) will increase coverage by 0.33%.
The diff coverage is 95.00%.

❗ Current head dec4434 differs from pull request most recent head c0d8931. Consider uploading reports for the commit c0d8931 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   83.33%   83.67%   +0.33%     
==========================================
  Files         163      163              
  Lines        7502     7496       -6     
==========================================
+ Hits         6252     6272      +20     
+ Misses       1250     1224      -26     
Impacted Files Coverage Δ
crates/env/src/api.rs 43.75% <ø> (ø)
crates/env/src/backend.rs 0.00% <ø> (ø)
crates/env/src/engine/off_chain/impls.rs 52.63% <0.00%> (ø)
crates/env/src/types.rs 44.44% <ø> (ø)
crates/lang/macro/src/lib.rs 100.00% <ø> (ø)
...lang/macro/tests/ui/chain_extension/E-01-simple.rs 2.50% <ø> (ø)
crates/lang/src/env_access.rs 27.27% <ø> (ø)
crates/env/src/engine/off_chain/db/chain_spec.rs 100.00% <100.00%> (ø)
crates/primitives/src/key.rs 82.27% <0.00%> (-0.23%) ⬇️
crates/storage/src/traits/impls/prims.rs 96.77% <0.00%> (-0.15%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cfbe64...c0d8931. Read the comment docs.

@cmichi
Copy link
Collaborator Author

cmichi commented May 22, 2021

@athei The comment is implemented, do you have anything else?

@athei
Copy link
Contributor

athei commented May 22, 2021

It was discussed here that no ink-unstable feature should be added. Maybe it should be removed.

@cmichi
Copy link
Collaborator Author

cmichi commented May 26, 2021

It was discussed here that no ink-unstable feature should be added. Maybe it should be removed.

I get a bit sweaty when thinking about making the current rent API accessible without unstable flag. The reason is that in my mind it's pretty certain that we will have another iteration over the current status and not having it behind ink-unstable would mean that we e.g. have to mention it in the next rc changelog. Wdyt @Robbepop?

@cmichi
Copy link
Collaborator Author

cmichi commented May 26, 2021

I thought a bit more about it and came to the conclusion that it's best to remove the unstable feature for rent stuff as well. My reasoning is that we are unstable in the rc state anyway and I also want to add examples soon and making them only available on unstable would make them more complicated. Also we might get community input by not hiding this behind a feature flag.

@cmichi cmichi merged commit cbf08a0 into master May 26, 2021
@cmichi cmichi deleted the cmichi-implement-seal-rent-params branch May 26, 2021 07:39
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.

4 participants