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.

Sign up for GitHub

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

Jump to bottom

[draft] Storage Deposit #6514

Closed
wants to merge 14 commits into from
Closed

[draft] Storage Deposit #6514

wants to merge 14 commits into from

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Feb 6, 2023

in preparation for exposing StateValue### Description

Test Plan

@msmouse msmouse changed the title trivial: StateView::get_state_value_bytes [draft] Storage Deposit Feb 6, 2023
@msmouse msmouse force-pushed the 0205alden_deposit branch 4 times, most recently from a854b49 to f1ecc6f Compare February 8, 2023 22:30
msmouse
msmouse commented Feb 9, 2023
View reviewed changes
aptos-move/framework/aptos-framework/sources/storage_deposit.move
let entry = vector::borrow(&slot_charges, i);
let coin = coin::extract_from_aggregatable_coin(&mut global_storage_deposit.deposit, entry.amount);
coin::deposit(entry.account, coin);
// FIXME(aldenhu): central events kills concurrency, probably need to augment Account with these events
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidiw ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that there'd be no way to know why you get refunds, I'm not sure a way around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that it's part of the gas, I'm thinking being obscure (just like other gas charges) is fine 🤡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh.. the refund won't be deducted from gas_used though..

Copy link
Contributor

Choose a reason for hiding this comment

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

With the movement toward objects, we could consider explicit user-owned objects for reimbursements, but there's a whole host of other issues that need to be addressed, such as opt-in, what to do if the user has only one -- do we have a framework to maximize parallelization..

@msmouse msmouse force-pushed the 0205alden_deposit branch 9 times, most recently from 7447ff9 to 084b912 Compare February 15, 2023 15:32
@msmouse
trivial: StateView::get_state_value_bytes
61c3261 
in preparation for exposing StateValue
@msmouse
expose StateValue via StateView::get_state_value()
0e28d3a
@msmouse
trivial: define feature flag to control stoarge slot metadata
73f71d0
@msmouse
refactor and fix resource group write op convertion
0fbd448 
We only had a "New mistakenly converted to Modification" issue for normal resources.
Modules and resource groups were never affected.
@msmouse
Add update StateValue and WriteOp to support state slot metadata trac…
b928a9f 
…king

* StateValue now has a new flavor which carries StateValueMetadata.
* similarly there is now WriteOp::CreationWithMetadata and friends
@msmouse
refactor: squash SessionOutput::into_change_set() into SessionExt::fi…
4bf3ee4 
…nish()

SessionOutput is removed entirely, SessionExt::finish() directly returns
ChangeSetExt
@msmouse
Make MoveResolverExt a StateView and ConfigStorage
2fd4dfa 
So that in the VM adapter and session it's possible to access on-chain
configs and state slot metadata
@msmouse msmouse force-pushed the 0205alden_deposit branch 2 times, most recently from 9863cb1 to e9e5328 Compare February 15, 2023 17:05
@vgao1996 vgao1996 mentioned this pull request Feb 17, 2023
Merged
msmouse
msmouse commented Feb 23, 2023
View reviewed changes
aptos-move/aptos-vm/src/move_vm_ext/session.rs

let existing_value_opt = self
.remote
.get_state_value(state_key)
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 need to access the metadata on the existing state value here. @zekun000 @gelash

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label May 4, 2023
@msmouse msmouse removed the Stale label May 5, 2023
@msmouse msmouse mentioned this pull request May 6, 2023
Closed
@github-actions
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Jul 21, 2023
@github-actions github-actions bot closed this Aug 6, 2023
@msmouse msmouse reopened this Aug 7, 2023
@github-actions github-actions bot removed the Stale label Aug 8, 2023
@msmouse msmouse closed this Aug 21, 2023
@msmouse msmouse deleted the 0205alden_deposit branch August 25, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidiw davidiw davidiw left review comments

@lightmark lightmark Awaiting requested review from lightmark lightmark will be requested when the pull request is marked ready for review lightmark is a code owner

@grao1991 grao1991 Awaiting requested review from grao1991 grao1991 will be requested when the pull request is marked ready for review grao1991 is a code owner

@JoshLind JoshLind Awaiting requested review from JoshLind JoshLind will be requested when the pull request is marked ready for review JoshLind is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario will be requested when the pull request is marked ready for review gregnazario is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin will be requested when the pull request is marked ready for review movekevin is a code owner

@gelash gelash Awaiting requested review from gelash gelash will be requested when the pull request is marked ready for review gelash is a code owner

@runtian-zhou runtian-zhou Awaiting requested review from runtian-zhou runtian-zhou will be requested when the pull request is marked ready for review runtian-zhou is a code owner

@sasha8 sasha8 Awaiting requested review from sasha8 sasha8 will be requested when the pull request is marked ready for review sasha8 is a code owner

@danielxiangzl danielxiangzl Awaiting requested review from danielxiangzl danielxiangzl will be requested when the pull request is marked ready for review danielxiangzl is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 will be requested when the pull request is marked ready for review zekun000 is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg will be requested when the pull request is marked ready for review wrwg is a code owner

@vgao1996 vgao1996 Awaiting requested review from vgao1996 vgao1996 will be requested when the pull request is marked ready for review vgao1996 is a code owner

@georgemitenkov georgemitenkov Awaiting requested review from georgemitenkov georgemitenkov will be requested when the pull request is marked ready for review georgemitenkov is a code owner

@banool banool Awaiting requested review from banool banool will be requested when the pull request is marked ready for review banool is a code owner

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan will be requested when the pull request is marked ready for review 0xmaayan is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@msmouse @davidiw @sionescu