-
Notifications
You must be signed in to change notification settings - Fork 254
fix(client-utils): lower the default fee to 0.015 BLD for smart wallet #12209
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
base: master
Are you sure you want to change the base?
Conversation
dckc
left a comment
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 suggest changing ymax-planner, not client-utils
| // TODO parameterize as part of https://github.com/Agoric/agoric-sdk/issues/5912 | ||
| const defaultFee: StdFee = { | ||
| amount: [{ denom: 'ubld', amount: '500000' }], // XXX enough? | ||
| amount: [{ denom: 'ubld', amount: '15000' }], // XXX enough? |
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'm afraid this breaks other clients.
I suggest that the planner should pass in an explicit fee rather than using the default.
turadg
left a comment
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 issue is about the planner, not the default. Are we sure we want to change the default?
| // TODO parameterize as part of https://github.com/Agoric/agoric-sdk/issues/5912 | ||
| const defaultFee: StdFee = { | ||
| amount: [{ denom: 'ubld', amount: '500000' }], // XXX enough? | ||
| amount: [{ denom: 'ubld', amount: '15000' }], // XXX enough? |
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.
This PR should only lower it to an amount that is enough.
| amount: [{ denom: 'ubld', amount: '15000' }], // XXX enough? | |
| amount: [{ denom: 'ubld', amount: '15000' }], |
| : never; | ||
| }; | ||
|
|
||
| // TODO parameterize as part of https://github.com/Agoric/agoric-sdk/issues/5912 |
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.
drive-by but this comment is obsolete
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.
Pull Request Overview
This PR reduces the default transaction fee for smart wallet operations from 0.5 BLD to 0.015 BLD, addressing the issue that the current fee is higher than necessary for practical use.
- Updated the default fee constant in the signing smart wallet kit
- Adjusted test expectations to match the new fee value
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/client-utils/src/signing-smart-wallet-kit.ts | Updated defaultFee amount from 500000 ubld (0.5 BLD) to 15000 ubld (0.015 BLD) |
| packages/client-utils/test/signing-smart-wallet-kit.test.ts | Updated test assertions to expect the new fee value of 15000 ubld in two test cases |
b0ab9e9 to
240ada4
Compare
7e92f98 to
e54275b
Compare
| }); | ||
|
|
||
| const result = await signingSmartWalletKit.executeOffer(action); | ||
| const result = await signingSmartWalletKit.executeOffer(action, smartWalletFee); |
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.
question
I only found the place where we're using executeOffer in resolver, but I was not able to find where we're using executeOffer under services/ymax-planner folder
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.
ymax-planner uses invokeEntry, wrapped in reflectWalletStore:
agoric-sdk/services/ymax-planner/src/engine.ts
Lines 343 to 352 in d8f6bce
| const planner = walletStore.get<PortfolioPlanner>('planner', { | |
| sendOnly: true, | |
| }); | |
| const { tx, id } = await planner.resolvePlan( | |
| portfolioId, | |
| flowId, | |
| steps, | |
| policyVersion, | |
| rebalanceCount, | |
| ); |
The fee could either be passed in to reflectWalletStore:
agoric-sdk/services/ymax-planner/src/main.ts
Lines 178 to 181 in d8f6bce
| const walletStore = reflectWalletStore(signingSmartWalletKit, { | |
| setTimeout, | |
| makeNonce: () => new Date(now()).toISOString(), | |
| }); |
Or another idea would be to add a signingSmartWalletKit.withFee(fee) method, much like cmdRunner.withEnv(...)
agoric-sdk/packages/pola-io/src/cmd.js
Line 70 in d8f6bce
| withEnv: env => |
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.
@LuqiPan I pushed a couple of commits; f6d30c8 extending wallet store reflection to support specifying a fee and f92e699 taking advantage of it in ymax-planner (but limited the extension to just fee while @dckc and I work out #12055 (comment) ). But feel free to tweak further as needed.
| proposal?: object; | ||
| }; | ||
|
|
||
| const smartWalletFee: StdFee = { |
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.
question
Is there a better file to put this constant in so both planner and resolver can use it?
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 expect the planner and resolver to become less integrated, not more. So this is fine.
closes: https://github.com/Agoric/agoric-private/issues/541
Description
Currently the default fee for smart wallet tx is 0.5 BLD, while in practice 0.015 BLD should be enough. This PR adjusts this parameter
Security Considerations
N/A
Scaling Considerations
This should reduce the rate at which planner is spending fees
Documentation Considerations
N/A
Testing Considerations
Updated unit tests
Upgrade Considerations
This should not impact upgrading