-
Notifications
You must be signed in to change notification settings - Fork 820
CairoByteArray #1469
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
CairoByteArray #1469
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.
Just few comments.
…Compiler + custom parser
…d narrow response type
* feat: reduce TX wait time, and provide known failed reasons based on TX flow * test: eventless write bytearray xtest * feat: uint static factory methods * refactor: cleanup * feat: missing primitive integers implemented as cairo types * chore: cleanup
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 comments I would emphasize relate to the usage of Buffer
, the rest are mostly concerned with tidying up.
expect(isShortString(shortStr)).toBe(true); | ||
}); | ||
|
||
test('should return true for short strings', () => { |
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.
Should this test be kept? Would it make sense to add an isASCII
check inside isShortString
and note that constraint in its description?
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.
isShortString is bugged. I left a note below for todo, it should be fixed or removed. But I tried to minimize the scope of this PR, as it already gets huge.
* @param hex string to check | ||
* @returns true if hex string | ||
*/ | ||
function isHexString(hex: string): boolean { |
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.
A hex check function is present in both encode
and num
.
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.
Yes circular issue so this was the simplest solution.
case isTypeBytes31(type): | ||
return encodeShortString(val.toString()); | ||
return parser.getRequestParser(type)(val); | ||
case CairoUint8.isAbiType(type): |
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.
Could have all the uint/int cases fall through to a shared return
. Same thing in the response parser.
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.
Good point, I also taught about this, but
It would break the pattern of enclosed class, so I would need to have a generic CairoInt class that only has this check, or a generic Inc Class that spawns all Int types. In any case, I leave it for some future PR where it would need to be taught true.
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.
Or some other solution...
|
||
/** | ||
* !! Main design decision: | ||
* Class can't extend GetTransactionReceiptResponse because it is union type |
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.
Since the union members from GetTransactionReceiptResponse
overlap in their properties, it might be worth trying implements SomeType
where SomeType
is:
{ [k in keyof GetTransactionReceiptResponse]: GetTransactionReceiptResponse[k] }
//or
Pick<GetTransactionReceiptResponse, keyof GetTransactionReceiptResponse>
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 tried and failed miserably, I see a solution in not having an error case, but for the scope of PR, i leave this to some other PR.
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.
Extending this way return type would have only common props and not base props spread from the Api, and that was the point of this refactor, to have those props visible to TS.
🎉 This PR is included in version 8.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation and Resolution
Fix: #1455
Fix: #1014
RPC version (if applicable)
Usage related changes
TODO
Development related changes
TODO
Checklist: