-
Notifications
You must be signed in to change notification settings - Fork 297
Chore(dev hub) entropy improvements #3031
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
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
|
||
<DynamicCodeBlock | ||
lang="solidity" | ||
code={`\ | ||
function generateAttributes(bytes32 randomNumber) internal { | ||
int256 strength = mapRandomNumber( | ||
keccak256(abi.encodePacked(randomNumber, "strength")), | ||
15, | ||
20 |
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.
feel free to ignore but I'm curious if this would work with just doing the triple quotes like ```solidity ....
Feels a bit cumbersome to have to use <DynamicCodeBlock .../> every time we write code
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.
this is the same thing that we were discussing yesterday @aditya520 as in don't see any difference in the look and feel of these two methods.
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.
This component support shiki styles. I want to experiment with them in future.
Nice! |
"error-codes", | ||
"[Example Applications](https://github.com/pyth-network/pyth-examples/tree/main/entropy)", | ||
"[Entropy Explorer](https://entropy-explorer.pyth.network/)", | ||
"[Fortuna API Reference](https://fortuna.dourolabs.app/docs/)", |
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.
I think the icons for these links don't clearly indicate that they open a new page. I think the up-right arrow should go after the text.
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.
I agree, but that is the fumadocs default behaviour. I would need to override that component byone of our component, which will take some time.
## Migration Guide | ||
|
||
If you're upgrading from Entropy v1 to v2: | ||
(TODO: Add links to the interface) |
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.
the content on this page needs some work imo
-
Multiple Request Variants -- this doesn't seem like a benefit. I think the real user benefit is "configurable gas limits". Forget about explaining the other variants of Request Callback -- no one cares about anything besides the default variant and the one that lets you set a gas limit.
-
Improved Fee structure -- this is wrong. these fee calculation methods correspond to request methods. Just delete this section
-
Entropy Debugger / Enhanced Callback Status -- theses should be the same item. Also should be called Entropy Explorer. Point here being that there's a tool that people can use to see the status of their request / callback.
-
Gas optimization -- i don't think we actually did this
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.
Enhanced callback status should be a separate section IMO. The new flag passed under callback event enables user to have a better safeguard against callback failures.
- [Foundry](https://book.getfoundry.sh/getting-started/installation) | ||
- [Node.js](https://nodejs.org/en/download) (version >= v18.0.0) | ||
- [Foundry](https://book.getfoundry.sh/getting-started/installation). | ||
- [Node](https://nodejs.org/en/download). Run `node -v{:js}` to confirm. You should get an output with version >= `v18.0.0`. |
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.
the {:js} postfix is appearing in the guide. this is true for all similar instances below
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.
Yeah, that needed to be fixed. @alexcambose I would need your help here.
```bash | ||
This will create a new directory in `coin-flip{:bash}` named `contracts/src`, which will contain the smart contract code. | ||
|
||
<DynamicCodeBlock |
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.
the copy icon on these should be visible by default. Not clear to the reader that these are easily copyable right now.
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.
Overall lgtm, I have some small suggestions.
/* Global typography: tighten line spacing in docs content */ | ||
:where(.prose) { | ||
line-height: 1.5; | ||
} | ||
|
||
:where(.prose) :where(p, li) { | ||
line-height: 1.5; | ||
} | ||
|
||
:where(.prose) :where(h1, h2, h3, h4, h5, h6) { | ||
line-height: 1.2; | ||
} |
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.
I very strongly recommend against setting global styles; instead we should be scoping styles to components.
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.
@alexcambose
Can you please fix this in your next PR?
Summary
Rationale
How has this been tested?