CheckpointDB's implementation of DB makes types too narrow

[MicaiahReid]

Because CheckpointDB implements DB, the types for the database supplied by the user are restricted to only those defined in DB (i.e. get, put, del, batch, copy). This forces the user to restrict the properties of their database instances, or to have as AnotherDatabaseType type assertions in their code.

Ideally, the DB interface should be used to enforce that whatever database is used by the user has, at a minimum, all of the properties defined by DB, but that the other properties of the user's database are passed through the type system so that they are available for consumption.

(I have a PR incoming that will attempt to implement this)

[faustbrian]

Would you have some example code that is causing issues? I have a database implementation in my project that has more methods implemented than the DB interface declares and the project builds fine. An interface just requires you to implement at least those methods but not only those methods.

If your issue is that CheckpointDB doesn't mirror your original database that diverges from DB then that seems like a expected and reasonable behaviour since CheckpointDB isn't supposed to act as a proxy to your underlying database other than caching data before calling a very specific set of functions. If you have any custom functionality that diverges from the standard DB interface then that is an application/implementation concern, not something the CheckpointDB should be concerned with.

[MicaiahReid]

@faustbrian thanks for the response!

You are correct that we are expecting the CheckpointDB to mirror the original database that diverges from DB. Or even more broadly, we are expecting the database returned from trie.database() (which returns a CheckpointDB) to have the types of the original database. I'm sure this would be valuable for many of your users. We'd like to be able to use the trie as the touch-point for accessing data from the database, but the types don't allow it:

import { DB, Trie } from "@ethereumjs/trie";
import { Level } from "level";

const DB_OPTS = {};
class MyDB extends Level<Buffer, Buffer> implements DB {
  constructor(location: string) {
    super(location, DB_OPTS);
  }

  copy() {
    return new MyDB(this.location);
  }
}

class MyTrie extends Trie {
  constructor(db: MyDB, root: Buffer) {
    super({ db, root, useRootPersistence: true, useKeyHashing: true });
  }
}

const myDB = new MyDB("");
const myTrie = new MyTrie(myDB, null);

myDB.iterator(); // all good
myTrie.database().db.iterator; // type error

Previously, we could use the database from the Trie as-is because the typescript interface was already a LevelUp interface (which was available on the ._leveldb property). In the @ethereumjs/trie@5 release the typescript interface was needlessly removed -- it wasn't actually removed from the runtime.

Sure, we could add some db property to MyTrie, but this is requiring a run-time solution for a type problem. However, if the types for Trie and CheckpointDB use generics, the type of the original database could be made available to your users when accessing via the trie.

[faustbrian]

That sounds like a leaky abstraction (which is always an awful idea) and the more sane solution would be to bind your database instance to a container or kind of shared access object in your application rather than expecting third-party libraries to mirror your application-specific interfaces and logic but I'm not a maintainer so not up to me to decide those things.

Also, the db property you're accessing is technically private but it isn't explicitly marked as such because the architecture of the trie package in general is still littered with leaky abstractions, meaning you are already accessing properties you really shouldn't be accessing. Store the database instance in your application if you want a sane implementation that isn't bound to third-party libraries and could break at any moment.

[holgerd77]

What's the state of this discussion? 🙂

@MicaiahReid is it an option to do the architectural changes @faustbrian recommended? Any other suggestions or alternative ways to go?

Happy to hear additional comments and weightings! 🤓 😀

[MicaiahReid]

Hi @holgerd77, @faustbrian,

We have managed to make the changes internally to get the types here to work.

Things still would be a little cleaner if the CheckpointDB used typescript generics to forward the user-defined types through. I started on a PR for this, but couldn't get it quite as clean as I'd hoped, so I didn't end up submitting it. If you ever plan on implementing those generics, I can submit the PR anyways as a starting point.

Thanks for the responses!

[acolytec3]

I believe this issue will be obsolete with the v7 releases. Feel free to reopen if I'm mistaken

[holgerd77]

@acolytec3 I am not so sure if this issue is really resolved in v7, I would not know how? I think we should nevertheless close here though, this has been stale long enough, seems not pressing enough to be picked up by someone.