-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[GraphQL][DotMove][3/n] Adds External
resolver
#18798
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
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.
Very nice -- main comment here is related to distancing ourselves from the idea that external resolution always talks to mainnet.
crates/sui-graphql-rpc/src/types/dot_move/named_move_package.rs
Outdated
Show resolved
Hide resolved
25128c4
to
ea3731f
Compare
6a308ea
to
dd5e5fa
Compare
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 good to go modulo nits, thanks @manolisliolios !
for k in mapping.keys() { | ||
// SAFETY: we inserted the keys in the mapping before. | ||
let idx = mapping.get(k).unwrap(); | ||
|
||
let Some(Some(bcs)) = names.get(&fetch_key(idx)) else { | ||
continue; | ||
}; | ||
|
||
let Some(bytes) = Base64::from_str(&bcs.value.bcs).ok() else { | ||
continue; | ||
}; | ||
|
||
let Some(app_record) = bcs::from_bytes::<AppRecord>(&bytes.0).ok() else { | ||
continue; | ||
}; | ||
|
||
// only insert the record if it is a valid `app_record` | ||
results.insert(k.clone(), app_record); | ||
} |
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.
Here's a more compact way to do this:
for k in mapping.keys() { | |
// SAFETY: we inserted the keys in the mapping before. | |
let idx = mapping.get(k).unwrap(); | |
let Some(Some(bcs)) = names.get(&fetch_key(idx)) else { | |
continue; | |
}; | |
let Some(bytes) = Base64::from_str(&bcs.value.bcs).ok() else { | |
continue; | |
}; | |
let Some(app_record) = bcs::from_bytes::<AppRecord>(&bytes.0).ok() else { | |
continue; | |
}; | |
// only insert the record if it is a valid `app_record` | |
results.insert(k.clone(), app_record); | |
} | |
let results = HashMap::from_iter( | |
mapping | |
.into_iter() | |
.filter_map(|k, idx| { | |
let bcs = names.get(&fetch_key(idx))??; | |
let Base64(bytes) = Base64::from_str(&bcs.value.bcs).ok()?; | |
let app_record: AppRecord = bcs::from_bytes(&bytes).ok()?; | |
Some((k, app_record)) | |
}) | |
); |
Although I wonder whether if some of these steps fail, you should be producing an error instead, rather than dropping the value? Not sure -- that only works if the error can't be triggered by user input (i.e. it's an internal error of some kind.
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.
Thanks, love the more compact approach.
We'll error out when we try to populate things in the "controllers". Since this data loader is doing a bulk operation here (so it's likely that it is serving multiple requests), I didn't want irrelevant requests to error out that's why we ignore singular name errors on this layer.
It's a different story for full request errors, there we indeed do error out.
crates/sui-graphql-rpc/src/types/dot_move/named_move_package.rs
Outdated
Show resolved
Hide resolved
crates/sui-graphql-rpc/src/types/dot_move/named_move_package.rs
Outdated
Show resolved
Hide resolved
(But do take a look at the thing about using |
bf2b163
to
6e4390a
Compare
dd5e5fa
to
ff47292
Compare
Description
Introduces the external resolution, which requests "names" from a different graphql node.
In reality, that'll be a mainnet RPC endpoint being used.
Test plan
e2e tests updated to start a secondary service that utilizes external resolution, and depend on that:
Stack
typeByName
#18797DotMove
resolution #18774Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.