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

Add option to refund unused deposit for set method #11

Merged

Conversation

agostbiro
Copy link
Contributor

Hi,

This PR adds an option to the set method to refund part of the deposit that covers unused storage to the caller (fixes #6).

Invariants

I think there are three invariants to maintain with this feature:

  1. No refunds for storage that is in use.
  2. No double spending: same deposit cannot be withdrawn multiple times.
  3. A caller cannot withdraw more than they deposited in the call.

I ensured the first two invariants by reusing the existing storage_withdraw implementation.

I ensured the third invariant by checking that we don't refund more than what was in the attached balance:

4e0581d#diff-bc8da1acc242fddbedebb84935fc8bb893ed6bb4044090dbed4b2e53fc817b02R153

Integration tests

I added an integration test suite to convince myself that the implementation is correct. If you want to keep the integration tests, I'm happy to add CI for them in a follow up PR.

Rust 1.69

I added a rust-toolchain.toml file to pin Rust 1.69 as nearcore doesn't support Rust 1.70 yet (more info: near/near-sdk-rs#1035)

Reviewing

I implemented this feature to gain more experience with NEAR smart contract development. As a result, in spite of my best efforts, it's possible that I overlooked something important, so please review carefully.

contract/src/api.rs Outdated Show resolved Hide resolved
Copy link

@frol frol left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me, but I’m not a code owner here to make any decisions

integration-tests/Cargo.toml Show resolved Hide resolved
@@ -0,0 +1,12 @@
# Integration Tests
Copy link

Choose a reason for hiding this comment

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

Was there some limitation using the standard Rust tests folder for integration tests?

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 question! I put the integration tests in a separate crate so that its build script can build the WASM contract before running the tests. We could potentially make this work if the integration tests lived in the contract crate, but it would be quite hacky as the build target for tests would be native, but the contract needs to be compiled with WASM target.

Regarding naming of the integration tests directory, I followed the pattern in create-near-app, but happy to adjust.

Note that there is a top level tests folder in the repo with a single noop test that is not part of a crate and is not a member of the workspace. Maybe we could remove that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the tests folder now in ae24bfe

@evgenykuzyakov
Copy link
Collaborator

Since we're pinning this to a 1.69.0 for Rust, it makes sense to update Rust version in build_release.sh to match it. Then rebuild it (requires linux machine or non M-mac).

Also, please update CHANGELOG.md and bump the version to 0.11.0

@agostbiro
Copy link
Contributor Author

Thanks for the review @evgenykuzyakov! I updated the Rust version in build_docker.sh, bumped the change log and built a new release contract. I tested the release contract by deploying it to the test net. Apologies for the delay -- I'm on an M1 mac and needed to get my hands on an x86 build server.

@agostbiro agostbiro closed this Jul 20, 2023
@agostbiro
Copy link
Contributor Author

Ooops closed by accident.

@agostbiro agostbiro reopened this Jul 20, 2023
@evgenykuzyakov evgenykuzyakov merged commit cc06d0c into NearSocial:master Aug 3, 2023
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.

Ability to return unused deposit for set
3 participants