[Onboard quorum driver to fullnode] 2/n add execute transaction end points #3866
Conversation
|
|
||
| pub struct FullNodeQuorumDriverApi { | ||
| pub quorum_driver: Arc<QuorumDriver<NetworkAuthorityClient>>, | ||
| pub module_cache: Arc<SyncModuleCache<ResolverWrapper<AuthorityStore>>>, |
There was a problem hiding this comment.
This one makes me a bit nervous, as it makes quorum driver dependent on the store.
Do we have a concrete use case where tx sender wants to see events in json format?
There was a problem hiding this comment.
good point. I don't really know but has increasingly started to think we just need bcs in the responses. Let's make an issue for this and move the discussion there - if we want to remove the parsed structs, we should phase them out entirely. cc @patrickkuo
longbowlu
force-pushed
the
quorum-driver-2
branch
from
bd16a77 to
3456b74
Compare
| /// Flag of the signature scheme that is used. | ||
| sig_scheme: SignatureScheme, | ||
| /// transaction signature, as base-64 encoded string | ||
| signature: Base64, |
There was a problem hiding this comment.
not sure if you've rebased recently, but be aware that signature is now part of the signed data, so this should perhaps be refactored to reflect that.
There was a problem hiding this comment.
initially I tried to use TransactionData v.s. this bunch of things but it did not work out for some doc reasons. If I recall correctly, something in move-lang repo is not compatible with how we generate our api docs and refuse to compile .. @patrickkuo do you have some ideas whether/how this can be fixed?
There was a problem hiding this comment.
The types in message.rs are not json/schema friendly, we have created another set of rpc types mirroring them, e.g. TransactionData <> SuiTransactionData, we can create a SuiSenderSignedData for this.
|
I'm curious what our story is about equivocation prevention as we move to deprecate gateway |
longbowlu
force-pushed
the
quorum-driver-2
branch
from
3456b74 to
014f4e6
Compare
Best I've heard is potentially resolving them at epoch change |
|
We will need to do this in gateway->sdk because a user should not be able to easily trigger equivocation through normal operations, and a user should be able to send multiple transactions in parallel that use different gas objects. |
|
But of course in the end at epoch boundary we will also be able to remove any object locks as well, but that's a last resort |
| pub transaction_effects_digest: TransactionEffectsDigest, | ||
| pub effects: SuiTransactionEffects, | ||
| /// authority signature information signed by the quorum of the validators. | ||
| pub auth_sign_info: AuthorityStrongQuorumSignInfo, |
There was a problem hiding this comment.
the transaction_effects_digest and auth_sign_info duplicate the fields in SuiCertifiedTransaction, which is also part of SuiExecuteTransactionResponse. Is there any reason why we need these here?
There was a problem hiding this comment.
there's a TODO TODO: Change to CertifiedTransactionEffects eventually. in line 364.
Created an issue to track: #4207
| // TODO: Change to CertifiedTransactionEffects eventually. | ||
| EffectsCert { | ||
| certificate: SuiCertifiedTransaction, | ||
| effects: SuiCertifiedTransactionEffects, |
There was a problem hiding this comment.
Can we add the timestamp_ms and parsed_data here as well, as in
sui/crates/sui-json-rpc-types/src/lib.rs
Line 310 in 11aa1f6
There was a problem hiding this comment.
does gateway return a timestamp for txn execution today? I'm curious where the timestamp is generated.
For this return struct, we are looking to add timestamp, it needs some extra work though
There was a problem hiding this comment.
does gateway return a timestamp for txn execution today?
No.
For this return struct, we are looking to add timestamp, it needs some extra work though
got it, parsed_data is higher pri than timestamp_ms in this case. Let me take a look
In this PR we add
execute_transactionendpoint in fullnode.QuorumDriverApisince itsexecution_transactionfunction's signature is different fromRpcGatewayApi, including param list and result typeNext:
see also:
#3778