Print scheduledRewards instead of totalEarnings. Closes #255
#259
Conversation
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
@juliangruber I'd like to discuss the high-level picture first. (1) I expect this is going to create confusion for Station Core users. For example, if I were running a bunch of Station Core instances, aggregating their metrics and rendering them in a dashboard, I would calculate my expected earnings as How can we clarify what the number in Either way, it's IMO important to explain this in our docs (the README file?). (2) The upside is that we avoid duplication. The downside is a slightly more complex integration and closing the door to move RPC API calls querying on-chain state from |
Good call! I agree this would be clearer. What about
I'm in favor of reading this from Station Core, so that we only have this logic in one place. |
SGTM! If we are not going to use this metric in Station Desktop, then does Station Core need to periodically emit it? An alternative to consider: add a new CLI command to find out what's the current amount. |
| const provider = new ethers.JsonRpcProvider( | ||
| 'https://api.node.glif.io/rpc/v1', | ||
| null, | ||
| { batchMaxCount: 1 } | ||
| ) | ||
| const contract = new ethers.Contract( | ||
| '0xaaef78eaf86dcf34f275288752e892424dda9341', | ||
| JSON.parse( | ||
| await fs.readFile( | ||
| fileURLToPath(new URL('./abi.json', import.meta.url)), | ||
| 'utf8' | ||
| ) | ||
| ), | ||
| provider | ||
| ) |
There was a problem hiding this comment.
I find it weird that it's the Zinnia module that handles querying the smart contract for scheduled rewards.
In my mind, this functionality is not specific to Zinnia (or Bacalhau or any other module) and, therefore, it belongs to Station Core itself.
What are the arguments in favour of placing this new logic here?
There was a problem hiding this comment.
This is a good question! My initial reasoning was that bacalhau doesn't use this, so it doesn't belong in core, above all modules. And who knows, maybe we will even add another trusted module in the future?
Otoh, it doesn't belong into Zinnia either, because it's the module's decision whether to use Meridian. ethers might even run in Zinnia, since for this we don't need to sign messages - or we could just use the jsonrpc endpoint directly - then we only need fetch. I think for this we need a way for Zinnia modules to log metrics.
There was a problem hiding this comment.
Fair enough. Please update our checklist for deploying the new Meridian version to remind us that we need to update this place in Station Core, too?
There was a problem hiding this comment.
Once |
|
@bajtos hindsight please. I would like to get this out as a UX improvement. I think the interface isn't going to change, only where the code lives - which we can address next week. |
Initially, I as indifferent: On one hand, removing the duplication is good. On the other hand, there is also value in moving all on-chain querying into the Station Desktop front-end so that we don't have to maintain so much Electron IPC stuff.
As I wrote this, I realised it may not be feasible to query "scheduled rewards" from the front-end if different modules have different smart contract ABIs and even different methods to query the balance. We may need to move this logic down into each Zinnia module, so that Station does not have any module-specific logic. In that case, it makes a lot of sense that Station Desktop should consume So, let's do it! |
Yeah, I agree. Opened a GH issue to implement this new feature in Zinnia: filecoin-station/zinnia#421 |
Actually, this requires more work besides Zinnia, I opened a higher-level issue to keep track of all subtasks. |
Closes #255