refactor: Hide Db/Tx lifetimes via Deref/Generics #148
Conversation
✅ Heimdall Review Status
|
BrianBland
force-pushed
the
brianbland/hide-lifetimes-via-deref
branch
from
c4e3901 to
20053ad
Compare
| let mut tx = db.begin_rw().unwrap(); | ||
| let mut tx = begin_rw(&db).unwrap(); | ||
| tx.set_account(AddressPath::for_address(address), Some(account1.clone())).unwrap(); | ||
|
|
||
| tx.commit().unwrap(); | ||
|
|
||
| let account2 = Account::new(456, U256::from(123), EMPTY_ROOT_HASH, KECCAK_EMPTY); | ||
| let mut tx = db.begin_rw().unwrap(); | ||
| let mut tx = begin_rw(&db).unwrap(); |
There was a problem hiding this comment.
Did this really have to change? Or is it just for testing this alternative pattern?
There was a problem hiding this comment.
Good catch, accidentally updated this one
| } | ||
| } | ||
|
|
||
| pub fn begin_ro<DB: Deref<Target = Database>>( |
There was a problem hiding this comment.
Nit: this could be an associated function, e.g. Database::begin_ro, although this will conflict with the existing begin_ro method. Just wanted to point this out; I'm not actually sure what's the most elegant syntax here (this can also be changed at a later time)
There was a problem hiding this comment.
Yes, we could choose to specifically use Database::begin_ro and Database::begin_rw, removing the existing instance methods. Happy to do that in a follow up, as it will change many more instances in tests
|
Review Error for and-cb @ 2025-07-24 22:12:32 UTC |
BrianBland
force-pushed
the
brianbland/hide-lifetimes-via-deref
branch
from
20053ad to
b5bde05
Compare
cb-heimdall
dismissed
jjtny1’s stale review
Approved review 2998944923 from jjtny1 is now dismissed due to new commit. Re-request for approval.
Proposed as a less-opinionated alternative to #145
This should support Transactions with any type that derefs to
Database, including standard borrows as well as Arcs.