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

Modify GasPrices storage to support multi-asset fees #1310

Closed
Valentine1898 opened this issue Jun 17, 2024 · 0 comments · Fixed by #1412
Closed

Modify GasPrices storage to support multi-asset fees #1310

Valentine1898 opened this issue Jun 17, 2024 · 0 comments · Fixed by #1412
Assignees
Labels
rpc Related to proto rpc services/methods

Comments

@Valentine1898
Copy link
Contributor

Valentine1898 commented Jun 17, 2024

You are right, GasPrices does contain an asset_id, I missed that because asset_id is never defined.
But the problem is that our GasPrices storage is deprecated and not suitable for multi-asset fees

Now we store GasPrices assuming that we can only have one record

GAS_PRICES: {
    key: 'gas_prices';
    value: Jsonified<GasPrices>;
  };

Instead, we should use the assetId as the key

GAS_PRICES: {
    key: Jsonified<Required<GasPrices>['assetId']['inner']>;
    value: Jsonified<GasPrices>;
  };

But in that case, I expect we'll run into an error, because pd purposely omits the assetId for native GasPrices, but that's bad for us, because undefined can't be a key in indexed-db

We will have to manually add the assetId for native GasPrices or come up with another trick

Also, note that the getGasPrices function will also have to be modified

async getGasPrices(): Promise<GasPrices | undefined> {
    const jsonGasPrices = await this.db.get('GAS_PRICES', 'gas_prices');
    if (!jsonGasPrices) return undefined;
    return GasPrices.fromJson(jsonGasPrices);
  }

Also need to change the gasPrices rpc for the view service so that it retrieves alt_gas_prices

And the block processor logic that should save alt_gas_prices to indexed-db when needed

Originally posted by @Valentine1898 in #1268 (comment)

@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra web Jun 17, 2024
@Valentine1898 Valentine1898 moved this from 🗄️ Backlog to 📝 Todo in Penumbra web Jun 17, 2024
@Valentine1898 Valentine1898 added the rpc Related to proto rpc services/methods label Jun 17, 2024
Valentine1898 added a commit that referenced this issue Jun 17, 2024
@TalDerei TalDerei self-assigned this Jun 17, 2024
TalDerei added a commit that referenced this issue Jun 17, 2024
* add index for spendable notes table in idb

* scaffolding planner support for multi-asset fees

* filter for alt fee asset id

* fix fee rendering

* full multi-asset fee support ~

* attempt to pass CI

* partially address comments

* valentine + jesse feedback

* fix broken lockfile, update deps, update db version

* remove dangling minifront_url

* co-locate idb version with storage package

* remove idb version from extension .env

* support actions for alt fees

* linting and organization cleanup

* update lockfile

* remove extension dir trace

* fix lockfile?

* address valentine comments

* fix test suite

* try fixing rust tests

* rust lint

* rust lint

* add TODOs for #1310

* fix lint

---------

Co-authored-by: valentine <valentyn1789@gmail.com>
@grod220 grod220 added this to Labs web Jun 20, 2024
@grod220 grod220 moved this to 📝 Todo in Labs web Jun 20, 2024
@github-project-automation github-project-automation bot moved this from 📝 Todo to ✅ Done in Penumbra web Jul 4, 2024
@github-project-automation github-project-automation bot moved this from 📝 Todo to ✅ Done in Labs web Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rpc Related to proto rpc services/methods
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants