Conversation
fa01adf to
308ee38
Compare
- FlowCreditMarketRebalancerV1 and FlowCreditMarketRebalancerPaidV1 contracts - RebalanceArchitecture.md with decoupled design and sequence diagrams - Mocks (SimpleSinkSource), test helpers (test_helpers_rebalance.cdc) and auto_balance_test
308ee38 to
23014b2
Compare
23014b2 to
11769e4
Compare
cadence/tests/transactions/rebalancer/add_paid_rebalancer_to_wrapped_position.cdc
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| access(self) view fun getPath(uuid: UInt64): StoragePath { | ||
| return StoragePath(identifier: "FCM.Rebalancer\(uuid)")! |
There was a problem hiding this comment.
include contract address, probably store it in self. scope
use FlowCreditMarket instead of FCM for ease of renaming in the nearest future
There was a problem hiding this comment.
As far as I understand Contracts don't have an address, only the account who holds the contract has an address but that wouldn't help with storage collisions?
But I added the Version to the Rebalancer.
There was a problem hiding this comment.
sorry for the confusion, yes, it should be the address where the contract is deployed, to avoid storage path collision if someone else deploys the same contract to a different address
There was a problem hiding this comment.
let path = self.getPath(uuid: rebalancer.uuid)
self.account.storage.save(<-rebalancer, to: path)Isn't it stored inside the account?
How would there be a collision?
zhangchiqing
left a comment
There was a problem hiding this comment.
A few minior suggestion.
Agree with Alex 's concern regarding the case of multiple rebalancers per single position.
…c reabalancers" This reverts commit d5da5a5.
1326ea8 to
31b6726
Compare
e8e3fa2 to
85683ba
Compare
9476537 to
6566beb
Compare
| access(all) view fun getPaidRebalancerPath( | ||
| uuid: UInt64, | ||
| ): StoragePath { | ||
| return StoragePath(identifier: "FlowCreditMarket.RebalancerPaidV1\(uuid)")! |
There was a problem hiding this comment.
add account address
it'd be easier to identify which contract it belongs to
| return StoragePath(identifier: "FlowCreditMarket.RebalancerPaidV1\(uuid)")! | |
| return StoragePath(identifier: "FlowCreditMarket.RebalancerPaidV1_\(self.account.address)_\(uuid)")! |
There was a problem hiding this comment.
Can we use a shorter name "FCMRebalancerPaidV1_(self.account.address)_(uuid)"?
There was a problem hiding this comment.
Actually, I'm a bit confused why do we need this method. This function is not being used anywhere, and we have a getPath method, but it's internal (access(self)). What's the different usage case for them?
If it's a public method to be used by other contracts, better add comments explaining the usage.
There was a problem hiding this comment.
it will be FlowALPRebalancerPaidV1_(self.account.address)_(uuid) after renaming
as I understood, this is in case someone wants to create their own rebalancer
There was a problem hiding this comment.
I don't fully understand how / when its needed so not sure what comment to put on it.
#131 (comment)
I don't think anyone else would create a paid rebalancer, as they would be then paying the fees for everyone (same as us)
jordanschalm
left a comment
There was a problem hiding this comment.
Sorry, I haven't reviewed the PR in full. Leaving a review to re-surface these comments from Josh that were marked as resolved:
Robust documentation is non-negotiable for the kind of high-assurance software we are building here. We need to make it as easy as possible for readers to understand the software's behaviour, and the reasoning behind that behaviour.
Including at least basic documentation for the vast majority of fields, functions, and types (what Josh was requesting in the comments) should be considered the minimum bar for new code.
|
I 100% agree. Which is why I added a markdown file describing the software's behaviour, and the reasoning behind that behaviour. I will try to make this a bit more detailed but if its still not clear or detailed enough I am happy to iterate. I added comments above all contract describing what they do in this architecture. Or do you want me to describe the behaviour of the functions in the overall architecture in their docstrings? |
589c609 to
ae8812c
Compare
|
@holyfuchs Thank you for the updates in 0eb479c. I think that covers the request for "documentation for the vast majority of fields, functions, and types".
Yes, exactly this. Consolidated documentation about how the system behaves as a whole is great and useful, and thank you for including it initially as contract-level documentation. The point I want to make is:
Obviously there is a balance to be struck, rules are made to be broken, etc, but I would really like for that to be the starting point. |
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Closes: #90
After reviewing PR #80, it became clear that the approach had some problems.
This PR aims to provide a cleaner and more maintainable solution.
Updated Rebalance Architecture
The core philosophy is decoupling: each component operates independently with the least privilege necessary.
The Supervisor is currently in the design phase (not yet implemented).
Key Principles
rebalancefunction.fixReschedule()call is idempotent and permissionless, ensuring the system can recover without complex auth.Rebalancer variants
There are two rebalancer types; they behave the same for triggering rebalances.
The paid rebalancer is otherwise the same: it holds a rebalance capability and runs on the same schedule/trigger model; only who pays and who controls config differ.
creating a position
sequenceDiagram actor anyone participant FCMHelper as FCM<br/>Helper participant FCM participant AB as Rebalancer participant Supervisor anyone->>FCMHelper: createPosition() FCMHelper->>FCM: createPosition() FCMHelper->>AB: createRebalancer(rebalanceCapability) FCMHelper->>Supervisor: supervise(publicCapability)while running
sequenceDiagram participant AB1 as AutoRebalancer1 participant FCM participant AB2 as AutoRebalancer2 participant SUP as Supervisor loop every x min AB1->>FCM: rebalance() end loop every y min AB2->>FCM: rebalance() end loop every z min SUP->>AB2: fixReschedule() SUP->>AB1: fixReschedule() end