Conversation
…for consistency across tests and implementations
…ty in code and configuration
…onsistency across tests
…istency in check_network and predictoor_stats modules
|
Doing a quick search in the code base I can see there are still multiple places where OCEAN token is mentioned, why aren't those updates as well? |
| " LOW OCEAN BALANCE!" | ||
| if ocean_bal < Eth(10).to_wei() and name != "trueval" | ||
| " LOW PREDICTION TOKEN BALANCE!" | ||
| if prediction_bal < Eth(10).to_wei() and name != "trueval" |
There was a problem hiding this comment.
Why do we refere to the stake token as prediction token here? I suggest we keep it consistent with the rest and name is to stake_token_bal
There was a problem hiding this comment.
Good catch, I first changed it to that then made it prediction token, missed this one.
Fixed now!
| - `trader` - buy aggregated predictions, then trade | ||
| - `sim` - experiments / simulation flow | ||
| - `payout` - OCEAN & ROSE payout | ||
| - `payout` - prediction & ROSE payout |
There was a problem hiding this comment.
One more thing left, I think here it should say staked_token or USDC
|
What is the github issue for this? Please link to it in the PR title, and the PR description. (And that issue should link to the overall epic.) If these github issue(s) do not exist, then please create them. |
|
(I just renamed this PR to be in line with the naming in the code changes itself. Prediction token -> stake token.) |
trentmc
left a comment
There was a problem hiding this comment.
I thought more about this. I don't think we should make it more general to stake_token.
Rather, just make it a hardcoded change from OCEAN to USDC.
Why: because the extra layer of indirection adds complexity & risk.
- Complexity in code changes. Eg munging "address.json" for
stake_token. Instead, "address.json" itself should simply hold the addresses forUSDC. Way way cleaner. - Complexity in review. If hardcoded
OCEAN->USDC, most changes would be trivial. But withstake_tokenthere's a bunch more logic. Also in general it makes the code harder to read. - Complexity in usage. READMEs should be saying
USDCnotstake_token. Etc etc. - This additional complexity increases risk. Predictoor is not the main focus right now. I don't want to accidentally break, and then lose weeks fixing it. KISS, get the thing done, move on.
Once this change is made, ping me, and I will do a more thorough review.
@KatunaNorbert changes elsewhere (webapp etc) should also be hardcoded USDC, not stake_token. Please update as needed.
…ub.com/oceanprotocol/pdr-backend into rename-ocean-token-to-prediction-token
trentmc
left a comment
There was a problem hiding this comment.
Getting close! My comments are all about minimizing the changes to minimize risk, so that it's just nearly only a character-for-character change of OCEAN -> USDC.
| stake_token = feed_contract1.get_stake_token() | ||
| assert stake_token == web3_pp.OCEAN_address | ||
| def test_get_USDC(feed_contract1, web3_pp): | ||
| USDC = feed_contract1.get_USDC() |
There was a problem hiding this comment.
If the contract previously had the function get_stake_token() then why change it here? Let's minimize the changes.
|
|
||
| mock_get_opf_addresses.return_value = { | ||
| "dfbuyer": "0xdfBuyerAddress", | ||
| "dfbuyer": "0x0000000000000000000000000000000000000000", |
There was a problem hiding this comment.
Keep it as 0xdfBuyerAddress if possible. It makes it more obvious.
There was a problem hiding this comment.
0xdfBuyerAddress throws an error in tests because the address is not valid
There was a problem hiding this comment.
OK. Then go ahead with the 0x00.. address I guess.
| def set_token(self, web3_pp): | ||
| stake_token = self.get_stake_token() | ||
| self.token = Token(web3_pp, stake_token) | ||
| USDC = self.get_USDC() |
There was a problem hiding this comment.
As above, if the smart contract already has get_stake_token() then let's leave it as that. Minimize rocking the boat.
|
|
||
| # approve | ||
| logger.info("Approve spend OCEAN: begin") | ||
| logger.info("Approve spend %s: begin", self.token_symbol) |
There was a problem hiding this comment.
Please remove the extra layer of indirection. Then: does it need self.token_symbol attribute at all? I want to minimize rocking the boat here.
| @arguments | ||
| predicted_value: The predicted value (True or False) | ||
| stake_amt: The amount of OCEAN to stake in prediction (in ETH, not wei) | ||
| stake_amt: The amount of tokens to stake in prediction (in ETH, not wei) |
| @description | ||
| Given an exact target # datatokens, calculates # basetokens | ||
| (OCEAN) needed to get it, and fee amounts too. | ||
| (tokens) needed to get it, and fee amounts too. |
|
|
||
| @return | ||
| baseTokenAmt_wei - # OCEAN needed, in wei | ||
| baseTokenAmt_wei - # tokens needed, in wei |
| logger.info("Checking account balances") | ||
|
|
||
| OCEAN = web3_pp.OCEAN_Token | ||
| USDC = web3_pp.USDC |
There was a problem hiding this comment.
USDC -> `USDC_token'. Minimize rocking the boat.
trentmc
left a comment
There was a problem hiding this comment.
LGTM! Thanks for responding to my earlier comments.
This pull request refactors the codebase to generalize the staking and payout token from a hardcoded "OCEAN" token to a hardcoded "USDC" token
CLI and Documentation Updates
claim_OCEANtoclaim_payouts, and updated documentation and argument parsing to reflect the new naming.Testing Refactor
USDCfixture and address, and ensured mocks and test logic are compatible with the new token abstraction.Code Improvements
token_symbolproperty toFeedContractfor improved logging clarity and reduced redundant contract calls.These changes collectively make the codebase more flexible, maintainable, and ready for supporting USDC