-
Notifications
You must be signed in to change notification settings - Fork 427
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
feat: Add new Sonic tokens, some SVM tooling drive-bys #5334
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5334 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
|
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.
lgtm but would also appreciate a second review from @daniel-savu on the rust improvements
assert!( | ||
self.description.is_some(), | ||
"Description must be provided for token with name: {}", | ||
self.name | ||
); | ||
assert!( | ||
self.image.is_some(), | ||
"Image must be provided for token with name: {}", | ||
self.name | ||
); |
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.
if these are mandatory, should their types have the Option
s removed?
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 it's a good point. My motivation was:
- Wanting to more closely match the actual metadata schema, and for us to just have more strict rules
- Iiuc this gives us some nicer error messages, whereas otherwise it may just give a generic serde error
- I just recently had a warp route I needed to deploy that didn't yet have an image, so I wanted to make it easy to override this and allow it to be optional if needed
Description
Drive-bys for the SVM tooling:
And adds the following tokens between Solana and Sonic: sSOL, USDStar, USDC, USDT, SONIC
Drive-by changes
Related issues
Backward compatibility
Testing