Skip to content

Auto generate JS safe types from Rust protoc types#702

Open
WilfredAlmeida wants to merge 39 commits intomasterfrom
STR-336
Open

Auto generate JS safe types from Rust protoc types#702
WilfredAlmeida wants to merge 39 commits intomasterfrom
STR-336

Conversation

@WilfredAlmeida
Copy link
Contributor

Fixes #665

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
…ypes

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
…osure

Signed-off-by: GitHub <noreply@github.com>
…terd types

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Copy link
Contributor

@leafaar leafaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need you to change home

lazy_static = "1.5.0"
prost-types = "~0.14"
solana-storage-proto = "3.1.6"
home = "=0.5.11"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not good, i would prefer we either using this on the worskspace but still no good, you can downgrade just using cargo update --precise so the Cargo.lock get the downgraded version.

Comment on lines 42 to +50
napi-build = "=2.2.4"
napi-sys = "=3.0.1"
syn = { version = "2", features = ["full"] }
quote = "1"
proc-macro2 = "1"
walkdir = "2.5.0"
napi-derive = "=3.3.0"
yellowstone-grpc-proto = "11.0.0"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you sure pinning makes sense?

readable: Arc<Mutex<UnboundedReceiver<SubscribeUpdate>>>,
writable: UnboundedSender<SubscribeRequest>,
/// Write side used by `write()`. Requests are forwarded to gRPC task.
writable: Arc<StdMutex<Option<UnboundedSender<SubscribeRequest>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why we need to have a mutex for this please?
UnboundedSender implements cheap Clone trait which reuse the same "channel" under the hood, allowing you to share.
Is the mutex a limitation of NAPI?

Copy link
Contributor

@lvboudre lvboudre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left couple of comments.
I wish there could be more unit test in typegen.rs, its alot of code without any test, its gonna be hard to maintain across team members.
I know testing code generation is not trivial, but I would like you give it a try with codex.
Could you ask him to review this file and generate unit test for you.

/// Write side used by `write()`. Requests are forwarded to gRPC task.
writable: Arc<StdMutex<Option<UnboundedSender<SubscribeRequest>>>>,
/// One-shot shutdown signal for the worker spawned by `subscribe()`.
shutdown_tx: Arc<StdMutex<Option<oneshot::Sender<()>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary?

The duplex stream has a receiver, when all receiver are dropped, you should be able to detect that in the background task that feed the channel via the SendError.

Comment on lines +102 to 122
// Explicit shutdown from JS destroy/close path.
_ = &mut shutdown_rx => {
break;
},

// 1. SubscribeRequest is received from self.write().
// 2. SubscribeRequest is propagated to Geyser client's sender.
req_option = writable_rx.recv() => {
if let Some(request) = req_option {
if let Err(e) = stream_tx.send(request).await {
if is_closing_worker.load(Ordering::Relaxed) {
break;
}
return Err(napi::Error::from_reason(e.to_string()))
}
} else {
// JS writable side dropped: no more requests can be sent.
// Exit worker so the upstream gRPC stream is torn down as well.
break;
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shutdown is redundant with the writable_Rx, wait for the channel to return an error to know when the loop should be shutdown.

Comment on lines +170 to +229
const TARGET_TYPES: &[&str] = &[
"ConfirmedBlock",
"ConfirmedTransaction",
"Transaction",
"Message",
"MessageHeader",
"MessageAddressTableLookup",
"TransactionStatusMeta",
"TransactionError",
"InnerInstructions",
"InnerInstruction",
"CompiledInstruction",
"TokenBalance",
"UiTokenAmount",
"ReturnData",
"RewardType",
"Reward",
"Rewards",
"UnixTimestamp",
"BlockHeight",
"NumPartitions",
"SubscribeRequest",
"SubscribeRequestFilterAccounts",
"SubscribeRequestFilterAccountsFilter",
"SubscribeRequestFilterAccountsFilterMemcmp",
"SubscribeRequestFilterAccountsFilterLamports",
"SubscribeRequestFilterSlots",
"SubscribeRequestFilterTransactions",
"SubscribeRequestFilterBlocks",
"SubscribeRequestFilterBlocksMeta",
"SubscribeRequestFilterEntry",
"SubscribeRequestAccountsDataSlice",
"SubscribeRequestPing",
"SubscribeUpdate",
"SubscribeUpdateAccount",
"SubscribeUpdateAccountInfo",
"SubscribeUpdateSlot",
"SubscribeUpdateTransaction",
"SubscribeUpdateTransactionInfo",
"SubscribeUpdateTransactionStatus",
"SubscribeUpdateBlock",
"SubscribeUpdateBlockMeta",
"SubscribeUpdateEntry",
"SubscribeUpdatePing",
"SubscribeUpdatePong",
"SubscribeReplayInfoRequest",
"SubscribeReplayInfoResponse",
"PingRequest",
"PongResponse",
"GetLatestBlockhashRequest",
"GetLatestBlockhashResponse",
"GetBlockHeightRequest",
"GetBlockHeightResponse",
"GetSlotRequest",
"GetSlotResponse",
"GetVersionRequest",
"GetVersionResponse",
"IsBlockhashValidRequest",
"IsBlockhashValidResponse",
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably read *.proto and do simple regex to grab all the types, this way it would make maintenance easier as we wouldn't to touch the code over and over again to add new types.

Here's a regex to capture all message|service|enum inside a proto :
(?m)^\s*(?:message|enum|service)\s+([A-Za-z_]\w*)\s*\{

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Error] Encode transaction error - SubscribeUpdateTransactionInfo.encode(data.transaction)

3 participants