-
Notifications
You must be signed in to change notification settings - Fork 208
chore: update devnet usdc mock #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to ecc55c7 in 1 minute and 43 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/oracle/utils.rs:7
- Draft comment:
Add a comment explaining why the USDC mock price is set to 0.0075. This will help clarify the rationale behind the update and its connection to devnet fee adjustments. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment is suggesting a code quality improvement - adding documentation for a magic number. However, it's making an assumption about the rationale ("devnet fee adjustments") that isn't evident from the diff itself. The comment is also somewhat speculative - it's assuming that documentation is needed without clear evidence that this value is confusing or that the rationale isn't obvious. Additionally, none of the other similar constants have explanatory comments, so this would be inconsistent. The rules state not to make comments that are "obvious or unimportant" and to avoid speculative comments. This seems like a minor documentation suggestion that may not be necessary. The comment could be valuable if this is a non-obvious magic number that will confuse future developers. Adding documentation for constants is generally good practice, especially when the value seems arbitrary. The suggestion is actionable and clear. While documentation can be helpful, this comment is making assumptions about the rationale ("devnet fee adjustments") that aren't supported by the diff. The tool doesn't have enough context to know why this value was chosen. Additionally, if every constant needed a comment explaining its value, the other constants would also need them. This feels like an "obvious or unimportant" suggestion that doesn't meet the bar for a required code change. This comment should be deleted. It's suggesting documentation for a constant value change without strong evidence that it's necessary. The rationale provided ("devnet fee adjustments") is speculative and not evident from the code. This falls under "obvious or unimportant" comments that don't require action.
Workflow ID: wflow_l8pUoUYDat9FdkeR
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 943467b in 1 minute and 14 seconds. Click for details.
- Reviewed
140lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/cli/src/args.rs:1
- Draft comment:
Removed the 'command' import; verify that #[command(name = "kora")] still works without the explicit import. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. crates/lib/src/fee/price.rs:148
- Draft comment:
Updated mock price tests: switched from 0.0001 to 0.0075 SOL per USDC and adjusted expected lamports accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing a change in the test values without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
3. crates/lib/src/token/token.rs:673
- Draft comment:
Updated USDC conversion rate in tests to 0.0075 SOL per USDC. Ensure all token value and fee calculations (including rounding behavior) are consistent with the new conversion. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all token value and fee calculations are consistent with the new conversion rate. This falls under the category of asking the author to double-check things, which is not allowed according to the rules.
Workflow ID: wflow_9yf8C3ooMINdfBDK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Not a huge issue, but when testing on Devnet w/ Devnet USDC the factor needs to not be too high otherwise fees exceed reasonable amounts one can get from the USDC Faucet. Example this 2M lamport fee comes out to be 23 USDC (more than can be fetched from the faucet.
Edit: updated a few unit tests that do math based on assumed devnet usdc value
closes PRO-586
Important
Update mock USDC price to 0.0075 SOL/USDC and adjust related calculations and tests.
utils.rs.price.rsandtoken.rsto reflect new USDC price.price.rsandtoken.rsto match new USDC price.price.rsandtoken.rsto align with updated price.commandfromargs.rs.This description was created by
for 943467b. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 81.0%
View Detailed Coverage Report