Batch methods modification #437
Conversation
| @@ -20,7 +20,7 @@ where | |||
| Lpns: Group, | |||
| Self: Into<LppBatch<LppRef<Lpn, Lpns>>>, | |||
| { | |||
| fn open_loan_req(&mut self, amount: Coin<Lpn>) -> Result<()>; | |||
| fn open_loan_req(self, amount: Coin<Lpn>) -> Result<Batch>; | |||
| fn open_loan_resp(&self, resp: Reply) -> Result<LoanResponse<Lpn>>; | |||
There was a problem hiding this comment.
for more consistent API please transform all the fn-s to consume its self
be aware that this would make obsolete more code - a debug assert at the calling location
In addition, the recent changes call for further simplification;
- remove the batch member variable at LppLenderStub
- remove the impl From<LppLenderStub<...>> for LppBatch
- ....
| } | ||
|
|
||
| lease_account.send(self.position.amount(), self.customer); | ||
| let updated_account = if !surplus.is_zero() { |
There was a problem hiding this comment.
too generic naming, please make it more specific
| fn migrate_leases<Leases, MsgFactory>( | ||
| &mut self, | ||
| mut self, |
There was a problem hiding this comment.
don't mutate self if you return a new instance, remove mut to check what is wrong
| Ok(MigrationStatus::CapacityReached(not_migrated_self)) | ||
| } | ||
| }, | ||
| Err(err) => Err(err), |
There was a problem hiding this comment.
Don't you think this calls for another construct that is more relevant here?
| struct MigrateBatch { | ||
| new_code: Code, | ||
| leases_left: MaxLeases, | ||
| msgs: Batch, | ||
| result: MigrationResult, | ||
| } | ||
|
|
||
| enum MigrationStatus { | ||
| CapacityReached(MigrateBatch), | ||
| Success(MigrateBatch), | ||
| } |
There was a problem hiding this comment.
the MigrationStatus smells
- used as a result of
MigrateBatch::migrate_leasesthat does not deal withnext_customerat all - used often with long delegation calls, for example:
state.result.msgsandmigrated_self.result.next_customer - multiple
matchexpressions
similarly, MigrateBatch's result is also a bad choice:
- it's mutated
- used in cases where it's not suitable
Please reconsider this design choice. As a first step, I'd suggest reverting the MigrateBatch to the previous member data and mutate-instead-of-consume logic to its operations.
…ownership of 'self'
maneva3
force-pushed
the
batch_methods_modification
branch
from
b74a4cb to
29b2dfb
Compare
No description provided.