Upward Unknown will be handled to raise an error #47

rbaconcordium · 2025-09-09T07:11:08Z

Jira-Id: COR-1780

Purpose

Enabling forward compatibility in transaction-logger with the recent changes on rust SDK

Changes

Ensuring function calls are handling Upward and if Upward.Unknown is encountered, an error will be raised. The insert_db overall would see this error and complain.
Have introduced also IndexingError, to wrap the different error types currently used within here, so that we could return at the higher level of the call. Using DatabaseError.Other does not appear to fit or could lead to inaccurate error display.

Checklist

[ X ] My code follows the style of this project.
[ X ] The code compiles without warnings.
[ X ] I have performed a self-review of the changes.
[ X ] I have documented my code, in particular the intent of the
hard-to-understand areas.
[ X ] (If necessary) I have updated the CHANGELOG.

Jira-Id: COR-1780

…nsactionOutcome>>

allanbrondum · 2025-09-09T11:36:44Z

src/main.rs

-            tx.query_opt(&self.insert_cti, &values).await?;
+        let unwrapped_upwards = match ts.summary.affected_contracts() {
+            Upward::Known(contracts) => contracts,
+            Upward::Unknown => Vec::new(), // TODO: is this what we want to do for Unknown?


I think clarifications at this level are best handle before the PR, e.g. in the squad channel. Such that we have the fundamental clarifications done before opening the PR.

But basically we should fail if any data we index on is unknown.

throwing err

allanbrondum · 2025-09-09T11:42:47Z

src/main.rs

+                }
+                Upward::Unknown => {
+                    //TODO: is this what we want to do for Unknown?
+                    println!(


Always use the logging framework (log create here)), never write directly to stdout

CHANGELOG.md

src/main.rs

allanbrondum · 2025-09-09T15:56:07Z

src/main.rs

-                &id,
-            ];
-            tx.query_opt(&self.insert_cti, &values).await?;
+        let upward_affected_contracts = match ts.summary.affected_contracts() {


I think you will better describe your intent here by using the projection BlockItemSummaryDetails::account_transaction and then call affected_contracts on that if not None.

allanbrondum · 2025-09-09T16:02:18Z

src/main.rs

+        };
+
+        for upward_contract_address in upward_affected_contracts {
+            match upward_contract_address {


Try using let affected = upward_contract_address.known_or_else(|| ...)?;, I think you will get a more clean looking code instead of using match. These functional combinators are often preferred in Rust

allanbrondum · 2025-09-09T16:05:51Z

src/main.rs

@@ -494,31 +519,42 @@ async fn get_last_block_height(
 ///
 /// The return value of [`None`] means there are no understandable CIS2 logs
 /// produced.
-fn get_cis2_events(bi: &BlockItemSummary) -> Option<Vec<(ContractAddress, Vec<cis2::Event>)>> {
+fn get_cis2_events(bi: &BlockItemSummary) -> Cis2Result {


I'm not convinced using type aliases specific to a function makes the code more readable. Maybe the type ContractEffects is ok, as it is not specific to a function, but Cis2Result is too much aliasing.

this one came about when the clippy command was run, complained about the type in the signature being too complex of some sort. Hence, why I broke them down to types. Will take a look again if I can lessen the number of types.

If you don't include Result and Option in the alias, I think it will be more readable, and clippy should not complain still.

have amendded

allanbrondum · 2025-09-09T16:09:33Z

src/main.rs

+            let affected_addresses = match summary.affected_addresses() {
+                Upward::Known(addresses) => addresses,
+                Upward::Unknown => Vec::new(), // returning empty vec if unknown, the next line will be processing zero length vec anyway, so nothing happens
+            };
+


This is not correct behaviour, since you will ignore transaction types that are unknown. And these transaction may actually affect accounts and we should index those effects then. You can project to account_transaction again here before calling affected_addresses, but you will need to fail when that function returns Unknown

adding an error throwing

throwing an error seems missing.

allanbrondum · 2025-09-09T16:13:11Z

src/main.rs

+                        Upward::Known(special_transaction_outcome) => {
+                            Ok(Some(special_transaction_outcome))
+                        }
+                        Upward::Unknown => Ok(None), // we ignore unknown special events, treat this as no special ones


Why ignore unknown special events? We may need to index them

changing to throwing an error

allanbrondum · 2025-09-09T16:15:54Z

src/main.rs

        None => {
-            let init = bi.contract_init()?;
-            let cis2 = init
+            let init = bi.contract_init().ok_or(DatabaseError::OtherError(anyhow!(


You changed the semantics here I believe? Before None was returned if contract_init() returned None, now it returns an Err

allanbrondum · 2025-09-09T16:17:07Z

src/main.rs

-                .ok()?;
-            Some(vec![(init.address, cis2)])
+                .collect::<Result<_, _>>()
+                .map_err(|_| {


allanbrondum

I think some of the implemented behaviour is not correct when considering the role of the indexer, as noted in the comments above.

In addition, it will help to document any decisions made in the PR description (here about how to handle Unknown, but also in general).

DOBEN · 2025-09-10T13:09:59Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 ## Unreleased changes

+- Added Upward usage in the transaction-logger service and throwing errors for unknown cases for affected_contracts


Please correctly highlight name/fileName/pathName/typeNames/functionName... as I already pointed that out in the previous two PRs. I think no need to mention transaction-logger service as this is the changelog about the transaction-logger (meaning redundant info).

DOBEN · 2025-09-10T13:21:13Z

src/main.rs

@@ -258,7 +263,7 @@ impl PreparedStatements {
        block_time: Timestamp,
        block_height: AbsoluteBlockHeight,
        ts: &BlockItemSummaryWithCanonicalAddresses,
-    ) -> Result<(), postgres::Error> {
+    ) -> Result<(), DatabaseError> {


As our Unkown error is not really a DatabaseError (the error doesn't come from the database), I would create a new error type e.g.:

/// A collection of possible errors that can happen while indexing blocks into the database. #[derive(Debug, Error)] pub enum IndexingError { /// Database error. #[error("Error using the database {0}.")] PostgresError(#[from] postgres::Error), /// ... #[error("Please update the rust SDK. Type {0} is unkown.")] Unkown(String), }

This general error case is a bit weird in my opinion (meaning an error for everything).

/// Other errors while processing database data. #[error("Error happened on database thread {0}.")] OtherError(#[from] anyhow::Error),

DOBEN · 2025-09-10T13:30:55Z

src/main.rs

+                            logs.iter().map(cis2::Event::try_from).collect();
+                        evs.map(|ev| (ca, ev)).map_err(|_| {
+                            DatabaseError::OtherError(anyhow!("Failed to parse CIS2 event"))
+                        })


This seems not a database error (meaning that a problem with the database exists). I am also not a fan of having everything be an otherError. I think it would be also good to propagate error messages such as e.

Suggested change

logs.iter().map(cis2::Event::try_from).collect();

evs.map(|ev| (ca, ev)).map_err(|_| {

DatabaseError::OtherError(anyhow!("Failed to parse CIS2 event"))

})

logs.iter().map(cis2::Event::try_from).collect();

evs.map(|ev| (ca, ev)).map_err(|e| {

IndexerError::Parsing(anyhow!("Failed to parse CIS2 event: {e} "))

})

DOBEN · 2025-09-10T13:35:13Z

src/main.rs

-        ),
+                .collect();
+
+            events.map(Some)


when does the function get_cis2_events return Ok(None)? An option as return value only makes sense if None is returned at some point?

DOBEN · 2025-09-10T13:35:23Z

src/main.rs

+                    DatabaseError::OtherError(anyhow!("failed to parse CIS2 event: {e}"))
+                })?;
+
+            Ok(Some(vec![(init.address, cis2)]))


when does the function get_cis2_events return Ok(None)? An option as return value only makes sense if None is returned at some point?

DOBEN · 2025-09-10T13:39:21Z

src/main.rs

+                .map(|upward| match upward {
+                    Upward::Known((ca, logs)) => {
+                        let evs: Result<Vec<_>, _> =
+                            logs.iter().map(cis2::Event::try_from).collect();


If you have transaction logs here (that are not smart contract cis2 logs) e.g. a contract not related to tokens, then you will try to parse the logs into cis2 events and fail the parsing and return an error which will stop the indexer from progressing. I don't think that is what we want.

DOBEN · 2025-09-10T13:44:06Z

src/main.rs

+            let affected_addresses = match summary.affected_addresses() {
+                Upward::Known(addresses) => addresses,
+                Upward::Unknown => Vec::new(), // returning empty vec if unknown, the next line will be processing zero length vec anyway, so nothing happens
+            };
+


throwing an error seems missing.

DOBEN · 2025-09-10T13:47:31Z

src/main.rs

+                        Upward::Known(special_transaction_outcome) => {
+                            Ok(Some(special_transaction_outcome))
+                        }
+                        Upward::Unknown => Err(Status::unknown(


This seems not a NodeError.

/// Error establishing connection. #[error("GRPC error during node query {0}.")] NetworkError(#[from] v2::Status), ```

also amended the general text of the errors IndexingError

td202 · 2025-09-12T11:33:41Z

CHANGELOG.md

+- Handling Upward Known and Unknown usage from main fn get_cis2_events
+- Handling Upward Known and Unknown usage from BlockItemSummary fn affected_contracts
+- Handling Upward Known and Unknown usage from Client fn get_block_special_events
+- Added IndexingError type in lib


I don't think this is a very useful changelog. I think it's best if the changelog focuses on behavioural changes, or what the effects of the changes are, rather than the implementation details, which are difficult to interpret without context. In this case, I think the relevant point is updating the Rust SDK for better forwards compatibility with future node versions, and revised error reporting for unknown transaction and event types.

td202 · 2025-09-12T11:58:15Z

src/lib.rs

+    /// Parsing errors.
+    #[error("Error happened parsing {0}.")]
+    ParsingError(String),


This doesn't seem to be used anywhere - what is it for?

td202 · 2025-09-12T12:02:44Z

src/lib.rs

+    #[error("Please update the rust SDK. Error could be due to {0}.")]
+    Unknown(String),
+    /// Other errors while processing database data.
+    #[error("Error happened on database thread {0}.")]
+    OtherError(#[from] anyhow::Error),


Some of these error descriptions are a bit vague or unhelpful.

td202 · 2025-09-12T12:08:35Z

src/main.rs

+        let upward_affected_contracts = ts.summary.affected_contracts().known_or_else(|| {
+            log::error!("Unknown upward error on ContractAddress {:?}", ts.summary);
+            IndexingError::Unknown("Unknown upward error on ContractAddress".to_string())
+        })?; // if Unknown, throw an error


An error occurs here if the BlockItemSummary is of an unknown type. A more appropriate error might be something like: "Could not determine affected contracts for a BlockItemSummary of unknown type."

td202 · 2025-09-12T12:11:59Z

src/main.rs

+        for upward_contract_address in upward_affected_contracts {
+            let affected = upward_contract_address.known_or_else(|| {
+                log::error!("Unknown upward error on ContractAddress {:?}", ts.summary);
+                IndexingError::Unknown("Unknown upward error on ContractAddress".to_string())
+            })?; // encountered unknown contract_address, throw an error to prevent transaction insertion


Here, the error would be due to an unknown AccountTransactionEffects type. "Could not determine affected contracts of an unknown type of AccountTransactionEffects."

td202 · 2025-09-12T12:28:52Z

src/main.rs

+                        return Err(IndexingError::Unknown(
+                            "Unknown error in contract_update_logs".to_string(),
+                        ));


This occurs if the contract invocation summary includes an unknown ContractTraceElement.

td202 · 2025-09-12T12:37:44Z

src/main.rs

+            let init = bi
+                .contract_init()
+                .ok_or_else(|| IndexingError::OtherError(anyhow!("Contract init failed")))?;


I don't think this is supposed to fail in this way. contract_init will return None if the event is not a contract initialization - in which case this function should also return None (or an empty vector).

td202 · 2025-09-12T12:47:49Z

src/main.rs

+                .filter_map(|ev| cis2::Event::try_from(ev).ok())
+                .collect();


It seems like this may be a change in behaviour - but possibly it was incorrect before. It appears that previously if any of the events was not a CIS2 event, then previously the function would return None, whereas now non-CIS2 events are ignored, but CIS2 events are retained. Probably this change should be documented if it's intentional.

td202 · 2025-09-12T12:50:54Z

src/main.rs

+                            Ok(Some(special_transaction_outcome))
+                        }
+                        Upward::Unknown => Err(Status::unknown(
+                            "Unknown upward error on SpecialTransactionOutcome",


Suggested change

"Unknown upward error on SpecialTransactionOutcome",

"Unknown SpecialTransactionOutcome type",

td202 · 2025-09-12T12:53:33Z

src/main.rs

-            let affected_addresses = summary.affected_addresses();
+            let affected_addresses = summary
+                .affected_addresses()
+                .known_or_else(|| Status::unknown("Unknown upward error on AccountAddress"))?; // if unknown, throw Err Status::unknown


Maybe the error should be something like "Could not determine affected addresses for BlockItem".

Richard B added 8 commits September 9, 2025 08:09

initial attempt of upward

8d322c7

Jira-Id: COR-1780

modified the call for affected_addresses to cater for Upward

495e1be

modified the call to affected_contracts to unwrap to the vector

cf51b63

modified the unwrapped for affected_contracts line

0715624

modified special_events, as it is now giving Vector<Upward<SpecialTra…

012ffef

…nsactionOutcome>>

Formatting correction

3384c24

ran clippy

63eb70e

committed deps folder

8f561cc

allanbrondum reviewed Sep 9, 2025

View reviewed changes

rbaconcordium marked this pull request as draft September 9, 2025 11:51

Richard B added 3 commits September 9, 2025 13:11

removed TODOs

0002b5c

throwing errors now on some cases

1613259

introduced types to avoid complex type as the output for get_cis2_events

43e6576

rbaconcordium marked this pull request as ready for review September 9, 2025 14:41

Amended changelog

4847e3f

rbaconcordium requested review from squirer and DOBEN September 9, 2025 14:50

squirer requested changes Sep 9, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

squirer reviewed Sep 9, 2025

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

Richard B added 2 commits September 9, 2025 16:14

amended based on feedbacks

08ef76f

amended changelog

a26e301

squirer self-requested a review September 9, 2025 15:35

squirer approved these changes Sep 9, 2025

View reviewed changes

allanbrondum reviewed Sep 9, 2025

View reviewed changes

src/main.rs Outdated

.ok()?;

Some(vec![(init.address, cis2)])

.collect::<Result<_, _>>()

.map_err(|_| {

Copy link

Contributor

allanbrondum Sep 9, 2025

Choose a reason for hiding this comment

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

Same here?

allanbrondum requested changes Sep 9, 2025

View reviewed changes

throwing errors after feedbacks

762bfa0

rbaconcordium changed the title ~~initial attempt of upward~~ Upward Unknown will be handled to raise an error Sep 10, 2025

amended the error text

4bd76d7

DOBEN reviewed Sep 10, 2025

View reviewed changes

Richard B added 6 commits September 10, 2025 17:13

introduced IndexerError and replaced the DatabaseError usages

bc59134

amended changelog

17667f9

amended the get_cis2_events to cater for None instead of error-ing

020566c

also amended the general text of the errors IndexingError

formatting issue resolved

a67e4e9

modified the error text

b4e2506

formatting issue

b69cd14

rbaconcordium requested a review from td202 September 11, 2025 15:35

rewrite the upward_affected_contracts section without using match

44e4e9e

td202 reviewed Sep 12, 2025

View reviewed changes

Richard B added 2 commits September 12, 2025 16:18

Amended changelogs and some error handling based on feedbacks

ec556c6

formatting issue fixed

110df7b

		@@ -2,6 +2,8 @@

		## Unreleased changes

		- Added Upward usage in the transaction-logger service and throwing errors for unknown cases for affected_contracts

	"Unknown upward error on SpecialTransactionOutcome",
	"Unknown SpecialTransactionOutcome type",

Upward Unknown will be handled to raise an error #47

Are you sure you want to change the base?

Upward Unknown will be handled to raise an error #47

Conversation

rbaconcordium commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

Checklist

Uh oh!

allanbrondum Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanbrondum Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanbrondum Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanbrondum left a comment

Choose a reason for hiding this comment

Uh oh!

DOBEN Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DOBEN Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DOBEN Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DOBEN Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbaconcordium commented Sep 9, 2025 •

edited

Loading

allanbrondum Sep 9, 2025 •

edited

Loading

allanbrondum Sep 9, 2025 •

edited

Loading

allanbrondum Sep 9, 2025 •

edited

Loading

DOBEN Sep 10, 2025 •

edited

Loading

DOBEN Sep 10, 2025 •

edited

Loading

DOBEN Sep 10, 2025 •

edited

Loading

DOBEN Sep 10, 2025 •

edited

Loading