chore(core, sequencer): move stateless checks to domain type parsing #1770
Conversation
| if timeout_time == 0 { | ||
| return Err(Ics20WithdrawalError::invalid_timeout_time()); | ||
| } | ||
| if amount == 0 { |
There was a problem hiding this comment.
Do we even need to check this? Protobuf represents 0 values as empty, and when decoding this it should result in None I think
There was a problem hiding this comment.
I don't even think we should reject this. If somebody wants to send (and pay for) a transfer, why would we not permit it?
There was a problem hiding this comment.
Removed in 84b00ef. I believe this has been a topic of discussion before, but that as of right now we don't allow empty transfers? I could be wrong, though.
| let parsed_withdrawal: Ics20WithdrawalFromRollup = | ||
| serde_json::from_str(&memo).map_err(Ics20WithdrawalError::parse_memo)?; |
There was a problem hiding this comment.
Should we edit our domain type Ics20Withdrawal to then include the parsed withdrawal as well? Then, we don't have to parse again during check_and_execute
There was a problem hiding this comment.
Yes, absolutely. Easiest is probably to just make Ics20Withdrawal an enum over 2 variants (one where bridge_address is set and contains the parsed payload, and one where it's not set and just contains the passed through String memo).
You should also lean on a serde(try_from = "") to a) first read into some UnverifiedIcs20WithdrawalFromRollup, and then b) via a TryFrom<UnverifiedIcs20WithdrawalFromRollup> Ics20WithdrawalFromRollup come up with the final type.
There was a problem hiding this comment.
Split into enum in 84b00ef. I wanted to ask further before continuing with your second suggestion: in order to do this we would need to implement a new UnverifiedIcs20WithdrawalFromRollup type and then the TryFrom trait as well for it. While this may be fine, it would be a little bit backwards from the way we currently do things.
Ics20WithdrawalFromRollup is the raw protobuf type, which usually represents the "unverified" version as opposed to the verified one. We could rename it to reflect that it is unverified, and then create a new domain type for the verified version, but I'm not sure if the name change would be proto breaking. In addition to this, it seems like it would be many more lines for questionable benefit to just performing these verifications in Ics20Withdrawal::try_from_raw. Do you still think it would be worth it to do this?
| if timeout_time == 0 { | ||
| return Err(Ics20WithdrawalError::invalid_timeout_time()); | ||
| } | ||
| if amount == 0 { |
There was a problem hiding this comment.
I don't even think we should reject this. If somebody wants to send (and pay for) a transfer, why would we not permit it?
| @@ -1107,8 +1136,20 @@ impl Protobuf for Ics20Withdrawal { | |||
| .transpose() | |||
| .map_err(Ics20WithdrawalError::invalid_bridge_address)?; | |||
|
|
|||
| if bridge_address.is_some() { | |||
There was a problem hiding this comment.
I am pretty unhappy that we now have a JSON formatted payload in here. I wonder if we can make a breaking change to these transactions and add another variant to the protobuf that makes this a protobuf.
There was a problem hiding this comment.
Considering this would be network breaking, I'm not sure what we can really do about it now, barring a hard fork :/
We could make a new action which uses a protobuf-formatted memo instead and follow the same upgrade path as the new ValidatorUpdateV2 action, but I'm not sure how worth it that would be.
| let parsed_withdrawal: Ics20WithdrawalFromRollup = | ||
| serde_json::from_str(&memo).map_err(Ics20WithdrawalError::parse_memo)?; |
There was a problem hiding this comment.
Yes, absolutely. Easiest is probably to just make Ics20Withdrawal an enum over 2 variants (one where bridge_address is set and contains the parsed payload, and one where it's not set and just contains the passed through String memo).
You should also lean on a serde(try_from = "") to a) first read into some UnverifiedIcs20WithdrawalFromRollup, and then b) via a TryFrom<UnverifiedIcs20WithdrawalFromRollup> Ics20WithdrawalFromRollup come up with the final type.
| @@ -37,6 +37,7 @@ penumbra-proto = { workspace = true } | |||
| prost = { workspace = true } | |||
| rand = { workspace = true } | |||
| serde = { workspace = true, features = ["derive"], optional = true } | |||
| serde_json = { workspace = true } | |||
There was a problem hiding this comment.
Will this change force us to always activate serde for all of astria-core? After all, the memo serde impls are feature gated I think.
There was a problem hiding this comment.
I tried to address this in 84b00ef. Let me know if this is what you had in mind!
github-actions
bot
added
conductor
Summary
Moved stateless checks for
Ics20WithdrawalandBridgeUnlockto occur when the types are parsed from protobuf.Background
Made more sense to check these fields sooner in the process rather than later, and be able to consolidate some of the checks that are shared between
BridgeUnlockandIcs20Withdrawal.Changes
Ics20WithdrawalandBridgeUnlockto profobuftry_from_rawandtry_from_raw_refimpls.Ics20Withdrawalto be an enum whose variants represent a user withdrawal and a withdrawal from rollup.Testing
Passing all tests.
Changelogs
No updates required.
Related Issues
closes #1430