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

Fix fees in WASM planner #3693

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix fees in WASM planner #3693

merged 1 commit into from
Jan 30, 2024

Conversation

jessepinho
Copy link
Contributor

@jessepinho jessepinho commented Jan 29, 2024

After some recent changes to the non-WASM/"regular" planner, the WASM planner was out of sync as regards fee handling. This PR gets it back in sync.

Manual testing (optional reading, but here for posterity)

This was hard to test because of some issues in Penumbra core. Steps I took to test it:

  • Rebase this PR onto commit 1d368d4 per this Discord discussion about a broken state in core's main branch
  • Set fees in crates/core/component/fee/src/genesis.rs:
          fn default() -> Self {
              Self {
                  fee_params: FeeParameters::default(),
     -            gas_prices: GasPrices::default(),
     +            gas_prices: GasPrices {
     +                block_space_price: 1,
     +                compact_block_space_price: 2,
     +                execution_price: 3,
     +                verification_price: 4,
     +            },
              }
          }
  • Compile the WASM planner (with this PR's changes included) to JS (cd crates/wasm/publish && pnpm install && pnpm run compile-wasm)
  • In the web mono-repo, check out my PR: Support fees in Penumbra web web#362
  • Install the newly built version of the WASM planner in the web monorepo (pnpm install ../penumbra-core/crate/wasm/publish/bundler; note that you first have to uninstall @penumbra-zone/wasm-bundler from both the root of the web monorepo and the packages/wasm directory)
  • Modify packages/constants/config.ts to set the grpcEndpoint to http:/localhost:8080

@jessepinho jessepinho force-pushed the jessepinho/web-362-wasm-planner-fixes branch from a782508 to 9dcaa94 Compare January 29, 2024 23:04
@@ -161,15 +161,23 @@ impl<R: RngCore + CryptoRng> Planner<R> {
///
/// This function should be called once.
pub fn add_gas_fees(&mut self) -> &mut Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes copied from

pub fn add_gas_fees(&mut self) -> &mut Self {
// Add a single Spend + Output to the minimum fee to cover paying the fee
let minimum_fee = self
.gas_prices
.fee(&(self.plan.gas_cost() + gas::output_gas_cost() + gas::spend_gas_cost()));
// Since paying the fee possibly requires adding additional Spends and Outputs
// to the transaction, which would then change the fee calculation, we multiply
// the fee here by a factor of 128 and then recalculate and capture the excess as
// change outputs.
//
// TODO: this is gross and depending on gas costs could make the gas overpayment
// ridiculously large (so large that the account may not have notes available to cover it)
// or too small. We may need a cyclical calculation of fees on the transaction plan,
// or a "simulated" transaction plan with infinite assets to calculate fees on before
// copying the exact fees to the real transaction.
let fee = Fee::from_staking_token_amount(minimum_fee * Amount::from(128u32));
self.balance -= fee.0;
self.plan.transaction_parameters.fee = fee.clone();
self
}

@jessepinho jessepinho changed the title WIP: Fix fees in WASM planner Fix fees in WASM planner Jan 29, 2024
@jessepinho jessepinho marked this pull request as ready for review January 29, 2024 23:44
@jessepinho jessepinho requested review from hdevalence and zbuc January 29, 2024 23:44
Copy link
Member

@zbuc zbuc left a comment

Choose a reason for hiding this comment

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

LGTM!

@zbuc zbuc merged commit 6cf8fcf into main Jan 30, 2024
7 checks passed
@zbuc zbuc deleted the jessepinho/web-362-wasm-planner-fixes branch January 30, 2024 16:22
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