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

LedgerDB namespacing #917

Merged
merged 6 commits into from
Jul 30, 2024
Merged

LedgerDB namespacing #917

merged 6 commits into from
Jul 30, 2024

Conversation

jfldde
Copy link
Contributor

@jfldde jfldde commented Jul 23, 2024

Description

  • Split up LedgerDB operations by usage
  • Introduces SharedLedgerOps for any method shared between Prover/Sequencer/Node.
  • Adds NodeLedgerOps, ProverLedgerOps and SequencerLedgerOps to constraint the associated CitreaFullNode, CitreaProver and CitreaSequencer

Linked Issues

@eyusufatik eyusufatik requested review from rakanalh and kpp July 25, 2024 15:09
Copy link
Contributor

@rakanalh rakanalh left a comment

Choose a reason for hiding this comment

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

I just gonna leave a comment here without "requesting changes" since i don't agree with the way things have been done here but i don't want to block the PR either.

The reason i have is:
We have Node,Prover,SequencerTrait... etc which are all implemented for LedgerDB which doesn't make that much of a difference from the previous version which LedgerDB had by containing all the methods. Limiting the access to methods using traits in prover for example by defining ledger_db: DB is okay.

The way i would have done it is as follows:
One trait, 3 structs

/// Common ledger ops, leaving out the word `common`/`shared` from the name.
pub trait Ledger {
  ... common stuff
}

pub struct ProverLedger {
}
impl ProverLedger {
}
impl LedgerOps for ProverLedger {
}

Same for sequencer and fullnode. I would also try to namespace tables

sequencer_xyz
prover_abc`

so that prover tables are only accessed within ProverLedger.

@jfldde
Copy link
Contributor Author

jfldde commented Jul 25, 2024

The main aim of this PR is to restrict visibility in order to prevent calling the method where it shouldn't be, i.e. calling a sequencer method from the prover.
I think we would have the same end result with the three structs you're describing.

If I understand correctly, we would declare the Ledger trait with default implementation that would then be implemented by the three structs (they have the exact same implementation for now).

For the namespacing of the tables, that would be ideal, but would it change anything if we're sharing the inner RocksDB instance ?

@jfldde
Copy link
Contributor Author

jfldde commented Jul 26, 2024

Isn't the three structs approach basically trying to do OOP where the Ledger trait would be the base class ?
Is that the paradigm we tend to follow ?
Curious on the approach we can take for namespacing the tables.

@jfldde jfldde marked this pull request as ready for review July 30, 2024 06:50
@eyusufatik eyusufatik merged commit 49d2f47 into nightly Jul 30, 2024
9 of 11 checks passed
@eyusufatik eyusufatik deleted the feature/ledgerdb_namespacing branch July 30, 2024 12:37
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.

LedgerDb namespacing for sequencer, node, prover
3 participants