-
Notifications
You must be signed in to change notification settings - Fork 72
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(sequencer): implement abci query for bridge account info #1189
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.
Can you please provide unit tests for these query handlers? Since we have written them as free standing functions unit testing should be relatively straight forward.
message BridgeAccountInfoResponse { | ||
uint64 height = 2; | ||
astria.primitive.v1.RollupId rollup_id = 3; | ||
optional bytes asset_id = 4; |
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.
What does it mean if this field is not set?
Also, I wonder if we should start using IBC denoms (i.e. strings) in their place - so this would be ibc/<hash>
. I think it would make things a bit neater.
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.
it means the account is not a bridge account - will add a comment to clarify
// A response containing the last tx hash given some bridge address, | ||
// if it exists. | ||
message BridgeAccountLastTxHashResponse { | ||
uint64 height = 2; | ||
optional bytes tx_hash = 3; | ||
} | ||
|
||
message BridgeAccountInfoResponse { |
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.
Small doc for what this is supposed to be the response to? I think this should mention ABCI because there is not (yet?) a matching grpc service.
} | ||
} | ||
|
||
impl raw::BridgeAccountInfoResponse { |
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.
It was pretty annoying having to update all these .into_native
/from_native
methods. I think I introduced these utilities prematurely and we should get rid of them/not introduce them for now (also since thehy are only used in astria-sequencer-client
).
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 these live under protocol
? They don't seem to be protocol-relevant?
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.
we can move them to a different protobuf package, can do in a follow-up
// containing the account information given some bridge address, | ||
// if it exists. | ||
message BridgeAccountInfoResponse { | ||
uint64 height = 2; |
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.
Why is this not indexed starting at 1? Is field number 1 reserved for something else?
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 matches the original response types which i think you wrote :p
// A response containing the balance of an account.
message BalanceResponse {
uint64 height = 2;
repeated AssetBalance balances = 3;
}
// A response containing the current nonce for an account.
message NonceResponse {
uint64 height = 2;
uint32 nonce = 3;
}
so not sure, we can change?
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, starting later is indeed weird. I wasn't aware that these are with us for a year already. :-S
In this PR, let them start at 1. The others we can fix in the followup breaking these request-response types out into a separate proto package.
code: AbciErrorCode, | ||
info: &str, | ||
) -> response::Query { | ||
if err.is_none() { |
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.
You can simplify the body of this function considerably by doing:
let log = match err {
Some(err) => format!("{info}: {err:?}"),
None => info.into(),
};
response::Query {
code: code.into(),
info: code.to_string(),
log,
..response::Query::default()
}
@@ -15,6 +18,153 @@ use crate::{ | |||
state_ext::StateReadExt as _, | |||
}; | |||
|
|||
fn error_query_response( |
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 like this helper.
) -> response::Query { | ||
use astria_core::protocol::bridge::v1alpha1::BridgeAccountInfoResponse; | ||
|
||
let address = match preprocess_request(¶ms) { |
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 think you can apply the ?
operator here because this method returns a response::Query
?
Also, other queries check the address prefix against the globally configured address prefixes, see the astria_sequencer::address
submodule.
Should this also do that?
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 think you can apply the ? operator here because this method returns a response::Query?
doesn't work since this function doesn't return a result, just a Query
:/
Should this also do that?
yes, will add
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.
doesn't work since this function doesn't return a result, just a Query :/
Ah, yeah. :S
fn error_query_response( | ||
err: Option<anyhow::Error>, | ||
code: AbciErrorCode, | ||
info: &str, |
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.
IMO rename this as msg
or something because the info
field in the query response (which is the human readable code
) and this info
argument tripped me up
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.
Also to add to this: I think you can forego the info
field entirely and lean on anyhow. If you encounter a None
, you can either None.context("something was not set").map_err(|e| error_to_query_response(e, <code>)
, or just match thing_that_can_none() { None => ... }
as before.
* main: (27 commits) refactor(sequencer): fix prepare proposal metrics (#1211) refactor(bridge-withdrawer): move generated contract bindings to crate (#1237) fix(sequencer) fix wrong metric and remove unused metric (#1240) feat(sequencer): implement transaction fee query (#1196) chore(cli)!: remove unmaintained rollup subcommand (#1235) release(sequencer): 0.14.1 patch release (#1233) feat(sequencer-utils): generate example genesis state (#1224) feat(sequencer): implement abci query for bridge account info (#1189) feat(charts): bridge-withdrawer, smoke test, various chart improvements (#1141) chore(charts): update for new geth update (#1226) chore(chart)!: dusk-8 chart version updates (#1223) release(conductor): fix conductor release version (#1222) release: dusk-8 versions (#1219) fix(core): revert `From` ed25519_consensus types for crypto mod (#1220) Refactor(chart, sequencer): restructure sequencer chart, adjust configs (#1193) refactor(withdrawer): read from block subscription stream and get events on each block (#1207) feat(core): implement with verification key for address builder and crypto improvements (#1218) feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs (#1209) chore(chart): update evm chart for new prefix field (#1214) chore: bump penumbra deps (#1216) ...
## Summary implement abci query for bridge account info, so users can tell if an account is a bridge account or not, and its relevant info if it is. ## Background needed for good UX ## Changes - implement abci query for bridge account info, so users can tell if an account is a bridge account or not, and its relevant info if it is - update sequencer client respectively ## Testing unit tests, sequencer client ## Related Issues closes #1118
Summary
implement abci query for bridge account info, so users can tell if an account is a bridge account or not, and its relevant info if it is.
Background
needed for good UX
Changes
Testing
unit tests, sequencer client
Related Issues
closes #1118