-
Notifications
You must be signed in to change notification settings - Fork 992
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
Fee unshielding #1281
Fee unshielding #1281
Conversation
d11b061
to
6a857ca
Compare
pls update wasm |
fe42313
to
ff23f8d
Compare
ff23f8d
to
dc947e0
Compare
3626bd5
to
6f89417
Compare
pls update wasm |
3a63665
to
abdcbe7
Compare
// from this function but try to take the funds from the unshielded | ||
// balance | ||
match apply_tx( | ||
TxType::Decrypted(DecryptedTx::Decrypted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here, I fake a decrypted tx to execute the unshielding. It's basically the same trick we use to execute governance proposals. At this point, I don't know whether we should think about allowing Raw
txs execution (making sure that no other Raw txs can end up in a block and be executed)
} | ||
if let Ok(TxType::Wrapper(_)) = process_tx(tx) { | ||
return Some(tx_bytes.clone()); | ||
match self.validate_wrapper_bytes(tx_bytes, &mut temp_block_gas_meter, block_time, &mut temp_wl_storage, &gas_table, &mut vp_wasm_cache, &mut tx_wasm_cache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gas check inside validate_wrapper_bytes
could actually be extracted from there and moved into the BlockSpaceAllocator
, effectively transforming it into a BlockSpaceGasAllocator
. It would probably make for a more coherent logic but would not improve performance. Actually, given that the allocator stops at the first element of the iterator that cannot be placed in the block, we could end up with blocks containing less transactions. Still, I like the idea of a SpaceGas
allocator better
@@ -146,31 +146,31 @@ pub async fn tx_signer( | |||
/// | |||
/// If it is a dry run, it is not put in a wrapper, but returned as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If it is a dry run, it is not put in a wrapper, but returned as is. | |
/// If it is a dry run, it is not put in a wrapper, but returned as is. | |
/// | |
/// If the tx fee is to be unshielded, it also returns the unshielding epoch. |
@@ -171,6 +173,8 @@ pub mod wrapper_tx { | |||
pub epoch: Epoch, | |||
/// Max amount of gas that can be used when executing the inner tx | |||
pub gas_limit: GasLimit, | |||
/// The optional, unencrypted, unshielding tx for fee payment | |||
pub unshield: Option<Tx>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rather than using the Tx
type here which allows arbitrary data and code, which would require some validation in the protocol (and also bloats the payload), it would be better to only attach masp Transaction
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be easy to implement given that we've already added the support for tx hashes in #1297. We can load the wasm code before running the operation. Still, some validation will be necessary even though we can get rid of the check on the content of the unshielding tx that will be done now with the type system
abdcbe7
to
c088cd9
Compare
pls update wasm |
1 similar comment
pls update wasm |
e550534
to
26e77c5
Compare
pls update wasm |
6b51c46
to
9870331
Compare
ab62f10
to
b9f7485
Compare
Closed in favor of #1327 |
Based on #1242.
Addresses #1010.
Adds an optional, unencrypted unshielding
Transaction
to theWrapperTx
to allow unshielding some tokens to pay fees when the transparent balance is insufficient.mempool_validate
,process_proposal
andprepare_proposal
finalize_block
when the wrapper is accepted