feat: keep track of DReps #128
Conversation
WalkthroughThis pull request makes extensive modifications across several crates by updating the public API for re-exports, transaction processing, ledger state management, and store handling. Changes include adding new types (such as Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jeluard
force-pushed
the
jeluard/dreps
branch
from
765d223 to
e90a756
Compare
| diff_bind.register(1, "a", None); | ||
| assert_eq!(true, diff_bind.unregistered.is_empty()); | ||
| assert_eq!(true, diff_bind.registered.contains_key(&1)); | ||
| assert_eq!(Some(&(None, Some("a"))), diff_bind.registered.get(&1)); |
There was a problem hiding this comment.
@KtorZ I would assume that this behavior of register value overriding bind implies we always expect register to come first?
If that's true, how about we make sure bind fails in case it's called first?
There was a problem hiding this comment.
Yes and no. We do expect register to come first, but it's also possible to bind without registering (because the registration might have happened in an earlier block!)
What is actually an error is calling register after bind. That is simply not allowed.
The difficulty here is that we are in-between state management and validation rules. Plus, we operate on a partial view of the system here since there might exist previous registrations or delegations in either of the volatile or stable dbs.
There was a problem hiding this comment.
(because the registration might have happened in an earlier block!)
Of course!
It makes me think that we should have a consistent error strategy here. It looks to me that some failing scenario might generate errors at different time depending on if happening in a single or multiple blocks (i.e. during initial block processing or delayed at the storage layer). e.g. registering the same pool multiple times.
There was a problem hiding this comment.
I would definitely treat any oddities like that in the storage layer as errors. Anything that the store receives should have been validated and the storage present our last safety net to check for consistency.
Yet, yes, you are right. Some errors are just "local to the transaction", while others depend on the ongoing state. In principle, those errors shall be what the ledger rules enforce and check. So, as we process the state, we might also yield validation errors unless we manage to treat the state changes as a non faillible builder API (driven by the ledger rules).
In which case, the validation rules become in fact the only place where we return validation failures whereas other places (state management or storage) are plain panics (wrapped in value errors, yes, but with a different semantic than the validation failures).
jeluard
force-pushed
the
jeluard/dreps
branch
2 times, most recently
from
4feced6 to
f8594ea
Compare
| } | ||
|
|
||
| pub fn bind(&mut self, k: K, j: Option<J>) { | ||
| assert!(!self.unregistered.contains(&k)); |
There was a problem hiding this comment.
We could use clippy to prevent assert usage vi this lint unfortunately it can't be configured to be usable in tests too. See rust-lang/rust-clippy#13790
There was a problem hiding this comment.
I suppose it's the eternal discussion between user errors and exceptions. Here, the asset was to signify an API misuse. It's a developer mistake to run into this problem; and it indicates that it is your job to check that you cannot bind on an unregistered element.
In this particular case, it's arguable. As we discussed, the line between state management and validation rules is really blurry. In a way, returning an error now makes this instrumental for implementation validation rules; whereas the assertion assumes that the invocations are valid.
| db.put(key, as_value(row)) | ||
| .map_err(|err| StoreError::Internal(err.into()))?; | ||
| } else { | ||
| error!( |
There was a problem hiding this comment.
@KtorZ Replicating the behavior from accounts now it appears to me that both always have a deposit? This logic derives from DiffBind storing values as Option.
There was a problem hiding this comment.
So technically:
- DReps always have an explicit deposits
- Accounts may have an explicit deposits; historically, they had not.
Although if I recall correctly how this was implemented in Amaru, we are defaulting to the protocol parameters' deposit value when the deposit is unspecified. So from a storage perspective, they should indeed always have a deposit.
That's a simplification we could do on the DiffBind structure; I am honestly not quite sure why it's still an option. Maybe trying to remove it will re-surface the reason why ^^"
| for (credential, (anchor, deposit)) in rows { | ||
| let key = as_key(&PREFIX, &credential); | ||
|
|
||
| // In case where a registration already exists, then we must only update the underlying |
There was a problem hiding this comment.
@KtorZ Looks to me that there would be some values in having a separated update methods in stores.
| @@ -304,12 +306,14 @@ impl Store for RocksDB { | |||
| utxo::add(&batch, add.utxo)?; | |||
| pools::add(&batch, add.pools)?; | |||
| accounts::add(&batch, add.accounts)?; | |||
| dreps::add(&batch, add.dreps)?; | |||
There was a problem hiding this comment.
All those separate changes to add new entities looks a bit brittle. There's tradeoff to be made if there's a need to introduce some more genericity, considering that the number of new entities is capped.
There was a problem hiding this comment.
It's an awkward trade-off indeed. On the one hand, it's straightforward to read, write and modify. There's nothing inherently complicated with this code and it doesn't really change that often. On the other hand, it's easy to forget to extend or change when we add new columns.
But the effect of forgetting anything here is immediately visible; you just won't get anything out of the app. So the risk of "shipping it wrong" is rather low.
Which is why I am not so interested in making this generic or more abstract. There are enough small subtleties between those methods to make a generic abstraction over it potentially way more complex than spelling them out.
I think two areas we could look into making generic / more abstract to avoid silly mistakes and catastrophic accident are:
-
The conversion from key objects to their prefixed rocksdb bytes identifier. Right now, you must call
as_keyvery religiously, otherwise you may end up storing things with a wrong key without actually noticing the damage until it's perhaps too late. This screams for aAsKeytrait that we require on each key type. -
Most of those function calls, as well as few others on epoch boundary MUST happen in a db transaction. Currently, they happen in multiple db transactions so the system is "easily" corruptible. So rather than hiding away the construction and commit of the db transaction, the API should make the transaction management explicit to the caller. We can still abstract away the RocksDB internal but we most likely need to reverse this responsibility here.
jeluard
force-pushed
the
jeluard/dreps
branch
2 times, most recently
from
fc9d79f to
3088609
Compare
crates/amaru-ledger/src/store.rs
Outdated
| >, | ||
| remove: Columns< | ||
| impl Iterator<Item = utxo::Key>, | ||
| impl Iterator<Item = (pools::Key, Epoch)>, | ||
| impl Iterator<Item = accounts::Key>, | ||
| impl Iterator<Item = dreps::Key>, | ||
| impl Iterator<Item = delegations::Key>, |
There was a problem hiding this comment.
Unused but required by type
jeluard
force-pushed
the
jeluard/dreps
branch
2 times, most recently
from
820890f to
91989a4
Compare
| let row = Row { | ||
| anchor, | ||
| deposit, | ||
| last_interaction: epoch.unwrap(), // Cannot fail; this is a new registration. |
There was a problem hiding this comment.
This is unfortunate. Will be probably improved in further PR working on governance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
crates/amaru-stores/src/rocksdb/columns/delegations.rs (1)
24-38: Function implementation looks good but could use more specific namingThe function logic is sound, but the name "add" is a tad generic for what it's doing. Since the comment says "Register a new DRep", might be worth renaming the function to something more descriptive like
register_dreporadd_delegationto make it crystal clear what's happening.Also, I see you're overwriting existing mappings without a check. If that's the intended behavior (which seems reasonable for updates), perhaps add a brief comment explaining the design decision.
- /// Register a new DRep. - pub fn add<DB>( + /// Register or update a delegation to a DRep. + /// Overwrites any existing delegation for the given credential. + pub fn add_delegation<DB>(crates/amaru-stores/src/rocksdb/columns/dreps.rs (1)
26-69: Implemented DRep registration function.Added the
addfunction to register or update DRep entries in the database. The function handles several cases:
- Updating an existing registration
- Creating a new registration with deposit
- Logging an error when attempting to register without a deposit
The implementation looks solid but has one potential issue: in line 55, there's an
unwrap()call with a comment stating it cannot fail. While this might be true based on current logic, it's generally better to use something likeunwrap_or(default_epoch)for future-proofing.- last_interaction: epoch.unwrap(), // Cannot fail; this is a new registration. + last_interaction: epoch.unwrap_or_else(|| panic!("Missing epoch for new DRep registration")), // More explicit about failure conditionsOr even better:
- last_interaction: epoch.unwrap(), // Cannot fail; this is a new registration. + last_interaction: epoch.expect("Missing epoch for new DRep registration"), // More descriptive errorThis would make the code a bit more robust to future changes, like a bloke with both belt and braces!
crates/amaru-ledger/src/state.rs (1)
88-96: parse_voting_procedures function
Neat approach to filter outDRepKeyvoters into aHashSet. Might be worth adding a quick test to ensure coverage.crates/amaru-ledger/src/store/columns/delegations.rs (1)
1-64: Good implementation of delegations storageThis new file looks well-structured and follows the established pattern for store columns. The Row structure with CBOR encoding/decoding looks solid.
However, there's a small copy-paste error in the error message:
- "unable to decode account from CBOR ({}): {e:?}", + "unable to decode delegation from CBOR ({}): {e:?}",The error message refers to "account" when it should be referencing "delegation" since this is in the delegations module.
monitoring/README.md (1)
186-188: New DRep trace events addedThe new trace events for DRep registration, unregistration, and updates align perfectly with the PR objectives. Consider adding detail sections for these new events, similar to what exists for the certificate events.
For consistency with other events, you might want to add detail sections like:
<details><summary>trace: `drep.registration`</summary> | field | description | | --- | --- | | `credential` | Stake credential being registered | </details>crates/amaru-ledger/src/state/volatile_db.rs (1)
232-233: Interesting approach to delegation managementI notice that while dreps have both registered and unregistered collections, delegations are only added (line 233) but never removed (line 240 uses an empty iterator). This matches your PR objectives which mention "delegations are treated as separate entities rather than attributes of DReps".
Is there a specific reason why delegations don't need to be explicitly removed? For future readers of this code, it might be worth adding a brief comment explaining this design decision.
delegations: self.state.delegations.into_iter(),- delegations: iter::empty(), + // Delegations are treated as separate entities and don't need to be explicitly removed. + // They are overwritten when updated or implicitly removed when the associated + // stake credential is deregistered. + delegations: iter::empty(),Also applies to: 239-240
crates/amaru-ledger/src/state/transaction.rs (1)
307-309: Don't forget the other certificate types!
The code quips “Ignore complex type certificates,” but theFIXMEsuggests incomplete logic. While flattening ensures simpler certificates, you’d still want to confirm no advanced certificate is left high and dry. A quick follow-up to handle additional certificate variants might be in order, so no stowaways remain unprocessed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/state.rs(5 hunks)crates/amaru-ledger/src/state/diff_bind.rs(2 hunks)crates/amaru-ledger/src/state/transaction.rs(4 hunks)crates/amaru-ledger/src/state/volatile_db.rs(6 hunks)crates/amaru-ledger/src/store.rs(3 hunks)crates/amaru-ledger/src/store/columns/delegations.rs(1 hunks)crates/amaru-ledger/src/store/columns/dreps.rs(1 hunks)crates/amaru-ledger/src/store/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/delegations.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/dreps.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/mod.rs(4 hunks)crates/amaru/src/bin/amaru/cmd/import.rs(5 hunks)examples/shared/src/store.rs(2 hunks)monitoring/README.md(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (50)
crates/amaru-stores/src/rocksdb/columns/mod.rs (1)
16-17: Excellent addition of new modules for governance support!The modules for delegations and DReps align perfectly with the PR's objective of implementing tracking mechanisms for DRep registration and delegation management. This modular approach will make future governance enhancements a piece of cake!
crates/amaru-ledger/src/store/columns/mod.rs (1)
16-17: Spot on with the ledger store modules!Brilliant to see the same structure mirrored here in the ledger crate. Keeping the organization consistent across crates is a proper stroke of genius - makes the codebase as easy to navigate as a well-marked London tube map!
crates/amaru-kernel/src/lib.rs (1)
38-42: Good updates to exported types for governance supportThe changes to the re-exported types from
pallas_primitives::conwaylook ace! AddingAnchor,GovActionId,Voter,VotingProcedure, andVotingProcedureswhile simplifying other exports sets the stage nicely for the governance features mentioned in the PR objectives.I notice that
VrfKeyhashappears to have been relocated rather than fully removed (it's still used in thePoolParamsstruct at line 177). Cheeky move, but all seems to be in working order!crates/amaru-stores/src/rocksdb/columns/delegations.rs (1)
21-22: Good prefix definition for delegation entriesUsing the UTF-8 encoding for "delg" makes for a clear and consistent prefix convention. As obvious as tea and crumpets!
crates/amaru/src/bin/amaru/cmd/import.rs (5)
29-32: Updated imports to support new DRep functionality.The imports have been updated to include BTreeSet and iter module, which will be used for the new DRep and delegation tracking functionality.
126-127: Added BTreeSet for voting DReps.A new parameter has been added to handle the collection of voting DReps in the save method. This aligns with the PR objective of tracking DReps.
355-361: Added support for DReps and delegations in UTxO import.Empty iterators for dreps and delegations have been added to the save method call, along with an empty BTreeSet for voting DReps. This ensures compatibility with the updated database schema.
Good to see a consistent approach across all the import functions. Like adding milk to tea - necessary for everything to work properly!
418-430: Added support for DReps and delegations in stake pools import.Empty iterators for dreps and delegations have been added to both the add and remove sections of the save method call in the import_stake_pools function, along with an empty BTreeSet for voting DReps.
508-514: Added support for DReps and delegations in accounts import.Empty iterators for dreps and delegations have been added to the save method call in the import_accounts function, along with an empty BTreeSet for voting DReps.
The pattern of adding these empty collections is consistent across all import functions. Well done on maintaining a uniform approach throughout the codebase!
examples/shared/src/store.rs (4)
1-2: Added imports for DRep functionality.Added necessary imports for BTreeSet and StakeCredential to support the new DRep tracking features.
119-130: Extended Store trait to support DReps and delegations.Added new iterator types for handling dreps and delegations in the save method. This enhancement allows the MemoryStore to track and process DRep registrations and delegations.
The structure follows the established pattern for other entity types like accounts and pools, making it a clean and consistent addition to the codebase.
136-138: Added remove support for DReps and delegations.Extended the remove parameter to support iterators for DRep and delegation keys that need to be removed from the store.
140-141: Added voting_dreps parameter.Added a new parameter to track a set of voting delegation credentials. This is a crucial addition for the governance functionality being implemented in this PR.
This parameter will be useful for monitoring which DReps are actively participating in voting, like keeping tabs on which MPs showed up to Parliament!
crates/amaru-ledger/src/state/diff_bind.rs (2)
53-66: Updated bind method to return Result.Modified the bind method to return Result<(), Error> and added proper error handling for the case when a key is already unregistered.
The handling of already unregistered keys is now explicit and clear, reducing the possibility of subtle bugs. Proper Aussie-rules error handling, mate!
74-113: Added comprehensive test coverage for DiffBind.Added a new test module with various test cases to validate the behavior of the register and bind methods under different conditions.
The tests cover different scenarios including:
- Register then bind
- Register with None then bind
- Bind then register (expecting failure)
- Bind only
This is brilliant test coverage! Like a good cricket match, you've covered all the bases (or wickets, rather).
crates/amaru-stores/src/rocksdb/columns/dreps.rs (2)
15-25: Defined DRep storage components.Added the necessary imports and defined the prefix used for storing DRep entries in the database.
The prefix is clearly defined with a comment explaining its purpose, and all the necessary imports are included. Good job on the documentation!
71-82: Implemented DRep removal function.Added the
removefunction to clear DRep registrations from the database. The function iterates through the provided keys and deletes the corresponding entries.Simple and straightforward implementation that follows the pattern established for other entity types. Handles errors appropriately by wrapping them in the StoreError::Internal variant.
crates/amaru-ledger/src/store/columns/dreps.rs (8)
1-14: All shipshape with the licence header
No big drama here, everything is in top form.
15-19: Imports are splendid
No cause for alarm, looks as breezy as a stroll by the Thames.
20-22: Clear doc comment
The doc comment is quite lucid, no suggestion from my side.
23-24: Value type definition
This typed tuple is straightforward and flexible. No issues.
25-26: Key type alias
A direct mapping fromStakeCredentialis grand. Keep calm and carry on.
27-32: Struct layout
The struct fields are well-chosen and make sense for storing deposit, anchor, and last interaction. Nicely done.
46-58: Encoding logic
Top-notch usage of CBOR encoding. No complaints from this corner of the pub.
60-69: Symmetrical decoding
Decoding matches the encoding structure perfectly, which is spot on.crates/amaru-ledger/src/state.rs (3)
27-30: Extended kernel imports
Lovely set of imports, capturing all required items. All is well in this corner.
35-35: New container import
BTreeSet and HashSet expansions look good for handling distinct sets of voting or DReps.
368-377: Inserting voting dReps
The logic for extractingvoting_drepsfrom the transaction body is straightforward. Double-check edge cases (like empty or malformedvoting_procedures) in an integrated test to avoid runtime surprises.crates/amaru-stores/src/rocksdb/mod.rs (8)
16-18: Adding epoch & stake-based imports
This seems dandy, bridging the kernel definitions for use in RocksDB.
31-31: Importing BTreeSet
This sets the stage for storing or iterating over sorted credentials. Good show.
267-268: Extra columns for DReps
Adding DReps into theColumnsiteration is a tidy approach. No fuss, no muss.
274-275: Removing DReps
Likewise, ensuring we can remove DReps is consistent. Looking shipshape.
278-278: Newvoting_drepsparameter
Brilliant: capturing the set of DReps used for voting ensures we've all relevant data on hand.
316-327: DReps epoch association
Neat technique to link newly added DReps with an epoch. Might be worth verifying that epoch values align with the rest of the system if there's ever an epoch boundary edge case.
328-329: Persisting DReps & delegations
Storing them in one batch is a time-saver. Everything looks grand.
336-336: DReps removal call
Ensures data remains consistent. No further ado required.crates/amaru-ledger/src/store.rs (4)
106-107: Support for DReps and delegations added to thesavemethodGood job adding the new parameters to support DReps and delegations in the
savemethod. This implementation aligns well with the PR objectives of initiating governance support and tracking DReps.Also applies to: 113-114, 117-117
166-172: New generic parameters for Columns structureNice addition of the new generic parameters
DandDLto theColumnsstruct to represent dreps and delegations. The structure maintains clean separation of concerns while extending functionality.
174-184: Updated Default implementation for ColumnsThe Default implementation has been properly updated to include empty iterators for the new dreps and delegations fields. This ensures backward compatibility with existing code that relies on the Default trait.
114-114: Unused but required by typeI see the previous comment about this line being "unused but required by type". This makes sense as it's needed for the proper type definition even if not directly used.
crates/amaru-ledger/src/store/columns/delegations.rs (1)
23-25: Clean type aliases for delegation storageUsing type aliases for
ValueandKeymakes the code much more maintainable. If you ever need to change the underlying types, you'll only need to update it in one place. Crisp as a packet of Tayto crisps!monitoring/README.md (4)
181-185: Reclassified severity levels to traceGood housekeeping - changing the severity levels from
debugtotracefor certificate events provides better granularity in monitoring. This will keep the logs cleaner while still preserving the ability to debug when needed.
239-240: Event severity and name updatesChanging
save.point_already_knownto trace level and renamingnew.found_snapshottonew.known_snapshotswith info level makes logical sense. These adjustments improve the signal-to-noise ratio in the logs.
288-289: Pool events severity updatedMoving these pool events from
debugtotraceis consistent with the other severity changes. The documentation remains clear and helpful.
181-181: Documentation generation reminderI see the previous discussion about investigating ways to generate this documentation automatically to prevent it from going out of sync. This remains a valid concern with the addition of new events.
crates/amaru-ledger/src/state/volatile_db.rs (3)
142-145: New fields added to VolatileStateThe addition of dreps, delegations, and voting_dreps fields to VolatileState is well-structured and aligns with the PR objectives. This implementation provides a clean way to track DRep registrations and delegations.
175-175: Added voting_dreps to StoreUpdateGood addition of the voting_dreps field to the StoreUpdate struct, ensuring that the DRep voting state is properly persisted.
207-207: Proper passing of voting_drepsGood job ensuring that voting_dreps from VolatileState is properly passed to the StoreUpdate. This is important for the correct handling of the DRep expiry tracking mentioned in the PR objectives.
crates/amaru-ledger/src/state/transaction.rs (2)
60-61: No worries! The logic for successful transaction IO looks grand.
You're replacing the transaction inputs and neatly enumerating outputs. This straightforward approach is quite readable, and it's a pleasant stroll in the code garden. Keep it up, mate!
162-164: Beware of potential negative fees!
Ifcollateral_returnhappens to exceed the total collateral,feeswill end up negative, which might be as welcome as a kangaroo in a crowded backpack. Consider adding a sanity check or a graceful handling mechanism to avoid calamities.
| pub fn register(&mut self, k: K, v: V, j: Option<J>) -> Result<(), Error> { | ||
| if self.registered.contains_key(&k) { | ||
| return Err(Error::KeyAlreadyUnregistered); | ||
| } | ||
| self.unregistered.remove(&k); | ||
| self.registered.insert(k, (j, Some(v))); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updated register method to return Result.
Modified the register method to return Result<(), Error> and added proper error handling for the case when a key is already registered.
One small issue though - there seems to be a mismatch in the error message:
- return Err(Error::KeyAlreadyUnregistered);
+ return Err(Error::KeyAlreadyRegistered);The error variant should be KeyAlreadyRegistered since we're checking if the key is already in the registered map.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn register(&mut self, k: K, v: V, j: Option<J>) -> Result<(), Error> { | |
| if self.registered.contains_key(&k) { | |
| return Err(Error::KeyAlreadyUnregistered); | |
| } | |
| self.unregistered.remove(&k); | |
| self.registered.insert(k, (j, Some(v))); | |
| Ok(()) | |
| } | |
| pub fn register(&mut self, k: K, v: V, j: Option<J>) -> Result<(), Error> { | |
| if self.registered.contains_key(&k) { | |
| - return Err(Error::KeyAlreadyUnregistered); | |
| + return Err(Error::KeyAlreadyRegistered); | |
| } | |
| self.unregistered.remove(&k); | |
| self.registered.insert(k, (j, Some(v))); | |
| Ok(()) | |
| } |
There was a problem hiding this comment.
Any reason why this isn't stored next to pool delegations inside the accounts column? I believe they fulfill the same role and relate to the same entities. So it would make more sense to extend the accounts column?
There was a problem hiding this comment.
Edit: This was based on a misunderstanding of the comment
delegations are a map that can be updated frequently irregardless of the account lifecycle. Storing them as part of an account would imply having to read/deserialize, update the map, serialize and finally update the whole key per delegation change.
I described a bit how I think the workflow could be achieved with this proposed layout (tradeoff being that there are some extra computations needed at startup).
Overall it looks like we could benefit some more structure for storage depending on overall entities lifecycle (what is updated when, what has to be computed when).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/amaru-ledger/src/state/diff_bind.rs (1)
44-51:⚠️ Potential issueWrong error variant used for already-registered keys.
At line 46, we throwKeyAlreadyUnregistereddespite the code block logically requiringKeyAlreadyRegistered. This is akin to mixing up your teabags and throwing in a coffee capsule instead—best to fix it lest we brew some chaos.Proposed fix:
- return Err(Error::KeyAlreadyUnregistered); + return Err(Error::KeyAlreadyRegistered);crates/amaru-ledger/src/state/transaction.rs (1)
302-305:⚠️ Potential issueUnwrap isn't necessary for HashMap.insert.
HashMap's
insertmethod returns the previous value if there was one, orNoneif there wasn't. Using.unwrap()here is unnecessary and could panic if we expect a previous entry that doesn't exist.- delegations.insert(credential, drep).unwrap(); + delegations.insert(credential, drep);This is like checking if your coffee cup explodes after putting coffee in it - when all you really need to do is just pour the coffee!
🧹 Nitpick comments (8)
crates/amaru-stores/src/rocksdb/columns/delegations.rs (2)
24-28: Documentation could use a bit more meat on the bones.The current docstring is a bit sparse like a Dublin pub at 9 am. Consider expanding it to include:
- What the function returns on success
- Any side effects to be aware of
- What happens if
rowsis empty- Parameter descriptions
- /// Register a new DRep. + /// Register new Delegated Representatives (DReps) in the database. + /// + /// # Parameters + /// * `db` - The RocksDB transaction to use for the operation + /// * `rows` - Iterator of (credential, DRep) pairs to be registered + /// + /// # Returns + /// * `Ok(())` - If all entries were successfully added + /// * `Err(StoreError)` - If any database operation fails + /// + /// # Note + /// This function will overwrite any existing mappings for the same credential. + /// If `rows` is empty, no changes will be made to the database.
33-34: Error handling looks good, but could be more specific.The current error handling wraps everything in a generic
StoreError::Internal. For a system tracking delegations in a governance context, it might be worth having more specific error variants. But it's a fair approach to start with!If you expand your error types in the future, consider something like:
- .map_err(|err| StoreError::Internal(err.into()))?; + .map_err(|err| StoreError::DelegationRegistrationFailed { + credential: credential.to_string(), + source: err.into() + })?;This would require adding a new variant to
StoreError, but would provide more context in error scenarios.crates/amaru-ledger/src/state/diff_bind.rs (3)
35-42: Ensure meaningful error messages.
Introducing a custom error type is grand; however, frankly, “KeyAlreadyRegistered” and “KeyAlreadyUnregistered” are so similar it might leave future devs doing a double-take. A wee doc comment or clarifying text in each variant could stave off confusion for the next poor soul rummaging through this code.
53-66: Clarify binding logic for unregistered keys.
bind(...)supports binding newly unregistered keys (pops them inEntry::Vacant). If that’s your intention, all good, mate. But do add a brief comment for future developers, lest we all scratch our heads in a fortnight.
74-113: Spot-check test coverage for edge cases.
The test suite’s looking spiffy, especially verifying that you can bind first then register. Yet, it might pay to test some corner cases like re-binding an unregistered key, or double-registering the same key and expecting an error.If you fancy, I can propose additional test stubs to cover these suspicious scenarios!
crates/amaru-ledger/src/state.rs (2)
88-97: Selective extraction of DRep keys.
parse_voting_procedureselegantly plops outStakeCredential::AddrKeyhashfor eachDRepKey. Ensure no other voter variants are left by the wayside if you plan to handle them in future.
368-375: Extend or store complete voting data?
At lines 368–375, only DRep credentials make it into the state. If you’ve a mind to track or process the entire set ofVotingProceduredetails, this might be the place. Otherwise, no worries, but do comment if further expansions loom on the horizon.crates/amaru-stores/src/rocksdb/mod.rs (1)
263-279: Augmented save() signature.
The newly introduced parameters for DReps and delegations insave(...)do make it a real mouthful. That said, it’s deliberate and clarifies usage. Bravo for the explicit design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/state.rs(5 hunks)crates/amaru-ledger/src/state/diff_bind.rs(2 hunks)crates/amaru-ledger/src/state/transaction.rs(4 hunks)crates/amaru-ledger/src/state/volatile_db.rs(6 hunks)crates/amaru-ledger/src/store.rs(3 hunks)crates/amaru-ledger/src/store/columns/delegations.rs(1 hunks)crates/amaru-ledger/src/store/columns/dreps.rs(1 hunks)crates/amaru-ledger/src/store/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/delegations.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/dreps.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/mod.rs(4 hunks)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs(5 hunks)examples/shared/src/store.rs(2 hunks)monitoring/README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/amaru-ledger/src/store/columns/mod.rs
- crates/amaru-stores/src/rocksdb/columns/mod.rs
- crates/amaru-kernel/src/lib.rs
- crates/amaru-stores/src/rocksdb/columns/dreps.rs
- examples/shared/src/store.rs
- crates/amaru-ledger/src/store/columns/dreps.rs
- monitoring/README.md
- crates/amaru-ledger/src/store/columns/delegations.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (31)
crates/amaru-stores/src/rocksdb/columns/delegations.rs (2)
21-22: PREFIX constant looks spot on!The constant is correctly defined as a byte array representing "delg" in UTF-8, which provides a clear namespace for delegation entries in the database. Good choice of prefix that's both meaningful and space-efficient.
31-32:✅ Verification successful
Design decision noted about overriding existing mappings.
The comment explicitly states that existing mappings will be overridden. This is a valid approach, just want to make sure it's the intended behavior for your delegation system. If you need to track history or prevent accidental overwrites, you might want a different strategy.
🏁 Script executed:
#!/bin/bash # Check if there are any similar override patterns or conflicting checks in other RocksDB operations echo "Searching for similar override patterns..." rg -A 5 -B 5 "Always override|override if|existing mapping" --type rust crates/amaru-stores/ echo "Checking if there are any get-before-put patterns that might indicate different approaches..." rg -A 5 -B 5 "db\.get.*db\.put" --type rust crates/amaru-stores/Length of output: 1442
Heads-up: The intentional override is spot-on!
The explicit comment and corresponding code confirm that existing mappings are meant to be overridden—just as planned. Our search didn’t reveal any conflicting implementations (like get-before-put strategies) elsewhere in the RocksDB operations, so it seems this approach is deliberate. If you ever need to keep a history of changes or prevent accidental overwrites, you might want to reconsider this strategy down the road. For now, carry on, and cheers to a clear design!
crates/amaru-ledger/src/state.rs (1)
27-35: Neat import expansions and BTreeSet usage.
Bringing inBTreeSet,HashSet, andVecDequeis a sensible call for the newly minted features. Keep a watchful eye on memory usage, though in many cases it’s as harmless as a tea break with biscuits.crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (5)
29-32: New imports for DReps and delegates.
Bringing inBTreeSetandHashMapis a sensible choice here. No issues spotted—just keep these imports pruned if usage changes.
126-127: Explicit placeholders for new store parameters.
PassingBTreeSet::new()todb.save(...)is a fine placeholder for delegations or DReps. Bodes well for future expansions—just remember to pipe actual sets in when ready.
[approve]
355-361: Recurrent usage of empty iterators.
Inimport_utxo, referencingiter::empty()for all new fields keeps the code uniform across different store columns. No immediate concerns, though if you foresee real data soon, keep an eye out for missed usage.
418-430: Maintaining consistency for stake pool imports.
Again,dreps: iter::empty()anddelegations: iter::empty()adhere to the updated store API. No drama there, cobber.
508-514: Accounts import hooking into new store fields.
This is consistent with the rest of the usage in this file. Good on you for your thoroughness— everything’s neat as a pin.crates/amaru-stores/src/rocksdb/mod.rs (3)
16-18: Imports for epoch and DReps.
Addingepoch_from_slot,Epoch, andBTreeSetpaves the way for DRep tracking in the store. All shipshape, chap.Also applies to: 31-31
316-330: Combining existing DReps with newly voted ones.
This block merges add.dreps withvoting_dreps. If a DRep’s in both, do we handle collisions or merges intentionally? Might be worth a smidge of documentation just in case.
336-337: DRep removal.
dreps::removedoes the job. No further complications spotted, mate.crates/amaru-ledger/src/state/volatile_db.rs (6)
142-146: Great additions to track DReps and delegations! 👍These changes effectively implement the main objective of tracking DReps and delegations as separate entities. The structure aligns nicely with the existing pattern in the codebase.
175-175: Clear addition for tracking voting DReps.The
voting_drepsset is a sensible addition to the StoreUpdate struct. This will help keep track of DReps that are actively participating in voting.
190-199: Types look spot on for state persistence.These iterator type updates properly extend the store update mechanism to handle the new DRep and delegation data. The signature maintains consistency with the existing pattern.
207-207: Proper transfer of voting_dreps state.Successfully passing the voting_dreps from the volatile state to the store update.
232-234: Good implementation of DRep state persistence.The code properly adds the registered DReps and delegations to the store update. This matches the PR's objective of tracking and persisting DRep information.
239-241:❓ Verification inconclusive
Consider handling delegation removals.
I notice that you're using an empty iterator for delegation removals. Is this intentional? It appears delegations can be updated but never removed from the store. If this is by design, brilliant! If not, you might want to track and handle delegation removals.
🏁 Script executed:
#!/bin/bash # Check if there's any code that removes delegations # Look for delegation removal in the codebase echo "Looking for code that handles delegation removal..." rg -A 3 "delegation.*remove|remove.*delegation" --type rust # Check if there are functions that might unregister delegations echo "Looking for potential unregister functions for delegations..." rg -A 3 "unregister.*delegation|delegation.*unregister" --type rust # Look for VoteDeleg certificate handling for removal echo "Looking for VoteDeleg certificate unregistration..." rg -A 10 "VoteDeleg.*unregister|unregister.*VoteDeleg" --type rustLength of output: 559
Delegation Removals: Intentional or Omission?
At present in
crates/amaru-ledger/src/state/volatile_db.rs(lines 239–241) we see delegation removals simply returning an empty iterator. I’ve run some queries—albeit with a dodgy output—to ascertain if the codebase handles removals elsewhere and found no evidence of it. This might be by design, but if delegation removal is required, you'll need to track and handle these similarly to how dreps are managed. It could also be helpful to leave a clarifying comment in the code if removals are intentionally unsupported.
- File:
crates/amaru-ledger/src/state/volatile_db.rs- Snippet in question:
dreps: self.state.dreps.unregistered.into_iter(), delegations: iter::empty(), },Could you please double-check whether the omission of delegation removals is deliberate or an oversight? This will help prevent any unintended side effects later on.
crates/amaru-ledger/src/store.rs (5)
106-108: Nicely structured additions for DRep and delegation data.These iterator parameters for adding DReps and delegations are well-specified and follow the established pattern in the codebase.
113-115: Good structure, but mind the unused parameter.The parameter for delegation removal keys appears to be intentionally unused based on the past review comment. This aligns with what we saw in
volatile_db.rswhere an empty iterator is used. If this is by design, no worries mate! But worth confirming if delegations should never be removed or if this is for a future implementation.
117-117: Good addition for tracking voting DReps.This parameter aligns with the changes in
volatile_db.rsand allows for the persistence of the voting DReps set.
166-172: Clean extension of the Columns struct.Adding the generic type parameters and fields for DReps and delegations follows the established pattern and maintains consistency.
174-186: Default implementation properly updated.The Default implementation has been correctly updated to initialize the new fields with empty iterators.
crates/amaru-ledger/src/state/transaction.rs (9)
77-80: Good updates to support DRep tracking.The updated parameters properly pass the DRep and delegation collections to the certificate application function.
98-134: Nice refactoring of transaction IO handling!Breaking out the successful transaction handling into a standalone function improves code clarity. The implementation correctly processes inputs and outputs while maintaining the same functionality.
140-192: Clean implementation of failed transaction handling.The failed transaction logic is neatly extracted and handles the collateral inputs/outputs correctly. The fee calculation is properly implemented as the difference between total collateral and return value.
194-225: Brilliant certificate flattening approach!This is a cracking solution to simplify certificate processing. Breaking down complex certificates into their atomic parts makes the rest of the certificate handling code more straightforward. Nice one!
232-233: Good additions for DRep handling.The updated parameter list for
apply_certificatesproperly includes the DRep and delegation collections.
290-293: Proper handling of DRep registration.The implementation correctly processes DRep registration certificates, storing both the credential and the anchor.
294-297: Proper handling of DRep unregistration.The implementation correctly processes DRep unregistration certificates.
298-301: Proper handling of DRep updates.The implementation correctly processes DRep update certificates, updating the anchor for the credential.
306-308: Good handling of flattened certificates.The code correctly ignores complex certificates since they've been flattened into simpler ones, and includes a FIXME comment for other certificate types. This aligns with the PR objectives of focusing on specific enhancements without extensive refactoring.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/amaru-stores/src/rocksdb/columns/dreps.rs (1)
61-66:⚠️ Potential issueWe're just logging when there's no deposit? That's a bit dodgy, mate!
When a DRep has no deposit, the code simply logs an error but doesn't throw an exception. This could lead to silent failures where a DRep isn't registered but the caller believes it was. Consider either:
- Returning an error to indicate the failure to register
- Using a default deposit value as mentioned by @KtorZ in a previous comment
This aligns with @KtorZ's point that "DReps always have an explicit deposits" and would make the behavior more predictable.
} else { error!( target: EVENT_TARGET, ?credential, "add.register_no_deposit", ) + return Err(StoreError::Internal("Cannot register DRep without deposit".into())); };
🧹 Nitpick comments (3)
crates/amaru-stores/src/rocksdb/columns/dreps.rs (3)
25-26: Crikey, a tiny typo in the doc comment!The comment says "Name prefixed used for..." which is a bit awkward. It should be "Name prefix used for..." instead.
-/// Name prefixed used for storing DReps entries. UTF-8 encoding for "drep" +/// Name prefix used for storing DReps entries. UTF-8 encoding for "drep"
72-73: Missing documentation - don't leave us in the dark!The
tickfunction is missing a doc comment explaining its purpose. Given that it's updating the "last_interaction" epoch for DReps, a good comment would help understand why and when this function should be called.+/// Updates the last interaction epoch for existing DRep registrations. +/// This helps track when DReps were last active, which is useful for expiry mechanisms. pub fn tick<DB>(
72-100: Good error logging, but consider making it more actionable.The error message "tick.unknown_drep" is a bit cryptic. Adding more context about what action the caller should take would make debugging easier. Also, similar to the
addfunction, you might want to return an error rather than just logging it.} else { error!( target: EVENT_TARGET, ?credential, - "tick.unknown_drep", + "tick.unknown_drep - Cannot update last interaction for a non-existent DRep. Register the DRep first.", ) + // Consider whether to return an error here, or continue with other credentials };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-stores/src/rocksdb/columns/dreps.rs(1 hunks)crates/amaru-stores/src/rocksdb/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru-stores/src/rocksdb/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (2)
crates/amaru-stores/src/rocksdb/columns/dreps.rs (2)
30-70: Bob's your uncle, but this function's doing two jobs!The
addfunction is handling both adding new entries and updating existing ones. While it works a treat, it might be clearer to split this into separate functions:
addfor new registrationsupdatefor modifying existing onesThis would make the code more maintainable and self-documenting. As our mate @jeluard mentioned in a previous comment, there would be "some values in having a separated
updatemethods in stores."/// Register a new DRep. #[allow(clippy::unwrap_used)] pub fn add<DB>( db: &Transaction<'_, DB>, rows: impl Iterator<Item = (Key, Value)>, epoch: Epoch, ) -> Result<(), StoreError> { for (credential, (anchor, deposit)) in rows { let key = as_key(&PREFIX, &credential); - // In case where a registration already exists, then we must only update the underlying - // entry. - if let Some(mut row) = db - .get(&key) - .map_err(|err| StoreError::Internal(err.into()))? - .map(Row::unsafe_decode) - { - row.anchor = anchor; - if let Some(deposit) = deposit { - row.deposit = deposit; - } - // Do not update the last interaction epoch as this is an existing DRep. - db.put(key, as_value(row)) - .map_err(|err| StoreError::Internal(err.into()))?; - } else if let Some(deposit) = deposit { + if db.get(&key).map_err(|err| StoreError::Internal(err.into()))?.is_some() { + update_one(db, credential, anchor, deposit)?; + } else if let Some(deposit) = deposit { let row = Row { anchor, deposit, last_interaction: epoch, }; db.put(key, as_value(row)) .map_err(|err| StoreError::Internal(err.into()))?; } else { error!( target: EVENT_TARGET, ?credential, "add.register_no_deposit", ) }; } Ok(()) } /// Update an existing DRep. pub fn update_one<DB>( db: &Transaction<'_, DB>, credential: Key, anchor: String, deposit: Option<u64>, ) -> Result<(), StoreError> { let key = as_key(&PREFIX, &credential); if let Some(mut row) = db .get(&key) .map_err(|err| StoreError::Internal(err.into()))? .map(Row::unsafe_decode) { row.anchor = anchor; if let Some(deposit_value) = deposit { row.deposit = deposit_value; } // Do not update the last interaction epoch as this is an existing DRep. db.put(key, as_value(row)) .map_err(|err| StoreError::Internal(err.into()))?; Ok(()) } else { error!( target: EVENT_TARGET, ?credential, "update.unknown_drep", ); Err(StoreError::Internal("DRep not found for update".into())) } }
102-113: Theremovefunction is simple and sweet - I like it!The delete operation is straightforward and handles errors properly. Top job!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/amaru-ledger/src/state/diff_bind.rs (1)
44-47:⚠️ Potential issueFix error variant in the register method!
There's a mismatch between the condition and the error variant. You're checking if the key exists in the registered map but returning KeyAlreadyUnregistered. This is like saying the pub's closed when actually it's too full!
- return Err(Error::KeyAlreadyUnregistered); + return Err(Error::KeyAlreadyRegistered);
🧹 Nitpick comments (4)
crates/amaru-ledger/src/store/columns/accounts.rs (1)
61-65: Properly updated CBOR encoding - brilliant!You've correctly updated the array size to 4 and added encoding for the drep field. The clone on drep might be unnecessary though - worth checking if that's needed or just a habit.
- e.encode_with(self.drep.clone(), ctx)?; + e.encode_with(&self.drep, ctx)?;crates/amaru-stores/src/rocksdb/mod.rs (1)
317-317: Consider adding a comment about DRep ticking functionalityThe
dreps::tickfunction is called with voting_dreps and epoch, but its purpose isn't immediately obvious from the name. A brief comment explaining that this updates the last interaction epoch for DReps would be helpful. If it's as clear as mud now, a comment would make it crystal!+ // Update the last interaction epoch for DReps that have voted in this epoch dreps::tick(&batch, voting_dreps, epoch)?;crates/amaru-ledger/src/state/volatile_db.rs (1)
138-139: Enhanced account structure.
You've updated theaccountsto store(Option<PoolId>, Option<DRep>)besidesLovelace. That’s a neat approach to represent multiple delegation targets—good on ya! Meanwhile, addingdrepsandvoting_drepsseparately is quite straightforward. Do watch out for potential duplication of logic. If you find yourself repeating the same insertion or merging patterns, it might be worth whittling that down in a future refactor.Also applies to: 141-141
crates/amaru-ledger/src/state/transaction.rs (1)
226-226: Prolonged reliance onunwrap().
Your newapply_certificateslogic looks grand. Mind, though, that repeatedunwrap()calls do carry risk. A wee bit more robust error handling might come in handy if, in future, you don’t want your ledger to keel over due to a single unexpected scenario.Also applies to: 230-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/amaru-ledger/src/state/diff_bind.rs(1 hunks)crates/amaru-ledger/src/state/transaction.rs(4 hunks)crates/amaru-ledger/src/state/volatile_db.rs(6 hunks)crates/amaru-ledger/src/store.rs(3 hunks)crates/amaru-ledger/src/store/columns/accounts.rs(3 hunks)crates/amaru-ledger/src/store/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/columns/accounts.rs(3 hunks)crates/amaru-stores/src/rocksdb/columns/mod.rs(1 hunks)crates/amaru-stores/src/rocksdb/mod.rs(4 hunks)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/amaru-ledger/src/store/columns/mod.rs
- crates/amaru-stores/src/rocksdb/columns/mod.rs
- crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs
- crates/amaru-ledger/src/store.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
🔇 Additional comments (23)
crates/amaru-stores/src/rocksdb/columns/accounts.rs (3)
30-33: Smooth addition of DRep handling - nice work!You've neatly updated the parameter unpacking to handle the extended value tuple, cleanly extracting both the delegatee and the new drep value. Good job on maintaining backward compatibility while extending functionality.
41-43: Well-implemented Row update logic!The drep field is properly assigned during row updates, ensuring consistent state management. This change properly integrates with the extended Row structure defined in the ledger crate.
49-54: Correctly extended Row initialization - spot on!The new drep field is properly included when creating a new Row, maintaining consistency with the updated Row structure. The implementation maintains the same pattern as the existing code, which is smashing!
crates/amaru-ledger/src/store/columns/accounts.rs (4)
15-15: Good import organization - clean as a whistle!The DRep type is properly imported from the kernel module, making the type available for use in the Row struct and Value type.
23-27: Well-structured Value type update!The Value type has been nicely extended to include both PoolId and DRep in a nested option tuple. This structure allows for flexible representation of delegation data.
35-35: Clean Row struct extension!Adding the drep field to the Row struct ensures delegation representation data can be properly stored and retrieved. Good job keeping it optional with Option.
76-76: Decoding properly handles the new field - ace work!The decode implementation has been updated to read the drep field from the encoded data, ensuring proper reconstruction of Row instances.
crates/amaru-ledger/src/state/diff_bind.rs (3)
35-41: Good use of thiserror for custom error handling!The Error enum with thiserror derive provides clear, well-structured error handling. This is a proper improvement over the previous implicit behavior.
53-66: Nice bind method enhancement with proper error handling!Good job updating the bind method to use a functional approach for updating the existing value and returning a proper Result. The error check for keys in the unregistered set is a solid improvement.
74-113: Comprehensive test coverage - top notch!Great job adding thorough tests that verify all the key scenarios: successful registration and binding, failure cases, and binding-only cases. These tests will help ensure the stability of this critical component.
crates/amaru-stores/src/rocksdb/mod.rs (4)
17-18: Good import additions - keeping it tidy!Adding StakeCredential and epoch_from_slot imports to support the new DRep functionality. Properly organized imports make the code more maintainable.
267-268: Well-structured parameter expansion!The save method signature has been neatly expanded to include iterators for DReps and a voting_dreps BTreeSet. This cleanly extends the functionality while maintaining the existing pattern.
Also applies to: 273-274, 276-277
314-318: Great addition of DRep epoch tracking!Deriving the epoch from the slot and using it for DRep tracking is exactly what the PR describes. This will enable proper expiry handling in the future. Nice one!
324-324: DRep removal properly handled!You've correctly added the DRep removal to match the addition logic, ensuring that DReps can be properly cleaned up when needed.
crates/amaru-ledger/src/state/volatile_db.rs (3)
18-19: Mighty imports, indeed!
AddingAnchorandDRepto the import list is a grand idea, especially with the new delegation bits. Just keep in mind that once you invite them to the party, you’ll want to ensure they’re actually used in a meaningful way across the codebase.
171-171: New fields inStoreUpdate.
Includingvoting_drepsin theStoreUpdatestructure, as well as the sets for newly introduceddreps, is well-aligned with your delegation-tracking objective. This addition is consistent and tidy, so no quibbles from me.Also applies to: 186-186, 192-192
201-201: Store update expansions.
Kudos for weavingvoting_drepsanddrepsinto theinto_store_updateflow. This ensures all new data is properly persisted (or removed) from the store. Nicely done, mate! Just be sure to watch for consistency of updates in case of complicated rollback scenarios.Also applies to: 226-226, 232-232
crates/amaru-ledger/src/state/transaction.rs (6)
20-20: New imports, new horizons.
Bringing inAnchor,DRep,BTreeMap, andBTreeSetsets the stage for your new delegation and anchor logic. This is a nice step forward for more advanced governance features.Also applies to: 25-26
53-53: Introduction of success or failure handling.
Swapping betweenapply_io_failedandapply_iofor handling transactions is a clean approach—clear as day and keeps things well compartmentalised. Just be mindful about future expansions if error-handling logic grows.Also applies to: 60-60
73-79: Certificate application call.
Passing new fields (pools,accounts, anddreps) toapply_certificatesensures your DRep logic flows nicely. Bravo for hooking everything up in one place. Top marks!
101-133: Solid function for successful transactions.
Theapply_iofunction is straightforward and efficient. Emitting new transaction outputs with an index referencing the transaction ID is spot on. Make sure your UTxO consumers all handle these indexes consistently.
139-191: Handling the rogues’ gallery of failed transactions.
Clever that you collect collateral in thefeesif no collateral return was set. This should deter the uninitiated from forgetting them. Just keep an eye on potential underflows if refunds might exceed the total collateral in some hypothetical scenario.
193-223: Flatten certificate wizardry.
Splitting complex certificates into multiple simpler ones is a cunning approach—like unraveling a big ball of yarn so your subsequent logic deals with normal, single-action certificates. Great for maintainability.
KtorZ
force-pushed
the
jeluard/dreps
branch
2 times, most recently
from
4bd5b6e to
b2581e4
Compare
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Due to trying to perform slot arithmetic on a point that is at the origin; used previously as a workaround by the import command to incrementally import snapshot. It is therefore time to get rid of that little hack and convey the intention properly with a flag. Signed-off-by: KtorZ <[email protected]>
…g accounts. This is a bit subtle, but since we might now generate account::add commands from either the delegation to a pool or a drep, we must ensure that storing a binding to one doesn't invalidate a binding to the other. The solution in this commit is only mildly satisfactory because it kind of pushes some of the state management logic to the store. A better way would be to perhaps not use Option but a more elaborate enum: 'Bind/Unbind/Unchanged' to clearly indicate whether something must be added, removed or simply left unchanged (even though the case 'unbind' will never occur in practice...) Signed-off-by: KtorZ <[email protected]>
The 'name:' field is actually pointless / ignored by the framework for reporting. Signed-off-by: KtorZ <[email protected]>
…napshots. Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
This is a first step towards full governance support. This PR goes straight to the point without introducing significant refactoring that are left for when the global picture will be better understood.
What's done:
DiffBinderror handlingepoch(to prepare for DRepexpiryhandling)Note that
voting_drepsis kept separate as it is expected that following PRs will significantly altervoteshandling.Note that generalized error handling is for now left out of this PR as long as there is no global strategy to tackle this.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
DRepsSummarystructure for managing delegations in governance.Bug Fixes
Tests
Documentation