Skip to content
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

Fix: Fixes for fetchDomainBalance lambda edge cases #3665

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Nov 7, 2024

Description

  • This PR aims to solve the following issues identified on QA
  • Cover case for action without token in getTotalFiatAmountFor - for this I have actually added a filter for the actions before reaching this step as it doesn't make sense to include the actions that don't have a token and an amount/networkFee
  • Use directly the network value for getting the default network token and network info in NetworkConfig - actually for this one I added a fallback to the network env var if the colony js mapping does not exist
  • Cover case for grouping actions by period, but the computed action period was not included in the initial setup - this was happening due to wrongly computing the start date of a period, including one extra day/month/week/year

Testing

TODO: Please test the fetchDomainBalance lambda and check these edge cases are covered.
Don't know how to easily do this on DEV without actually changing directly into code the used values as in for eg. adding a null token or using arbitrumOne as a value for the network env var etc. So let's do this 🦾

Important

To check the values returned or logged in a lambda, you need to always run the getDomainBalance query and then check the amplify logs

Step 1. Let's make sure the query runs as expected first

query MyQuery {
  getDomainBalance(
    input: {
      colonyAddress: "<COLONY_ADDRESS_HERE>", 
      timeframePeriod: 1, 
      timeframeType: MONTHLY
    }
  ) {
    total
    totalIn
    totalOut
  }
}

Step 2. Now let's try to break it by using a negative or 0 timeframePeriod

query MyQuery {
  getDomainBalance(
    input: {
      colonyAddress: "<COLONY_ADDRESS_HERE>", 
      timeframePeriod: -10, 
      timeframeType: MONTHLY
    }
  ) {
    total
    totalIn
    totalOut
  }
}

Step 3. For the next queries please change back the timeframePeriod to a value > 0

Step 4. Go to amplify/backend/function/fetchDomainBalance/src/config/envVars.js and replace
let network = Network.Custom; with
let network = 'arbitrumOne'

Step 5. Now in amplify/backend/function/fetchDomainBalance/src/config/networkConfig.js check the DEFAULT_NETWORK_TOKEN and DEFAULT_NETWORK_INFO

Step 6. Go to amplify/backend/function/fetchDomainBalance/src/utils.js go to getFormattedActions and replace the code with

const getFormattedActions = (actions, domainId) => {
  // Separating the actions created as extension support
  let extensionSupportActions = actions.filter(
    (action) => !!action.initiatorExtension?.id,
  );

  return actions
    .filter((action) => !action.initiatorExtension?.id)
    .map((action) => getActionWithFinalizedDate(action))
    .filter((action) => !!action.finalizedDate)
    .map((action) => {
      let amount = action.amount;
      let networkFee = action.networkFee;

      /**
       * If there is no domain selected (aka we are at colony level) and the action type is not among payments, we'll consider the amount to be '0'
       * Though we might need to reconsider this when transferring funds between colonies
       */
      if (!domainId && !paymentActionTypes.includes(action.type)) {
        amount = '0';
      }

      const attachedAction = extensionSupportActions.find(
        (extensionSupportAction) =>
          extensionSupportAction.rootHash === action.rootHash,
      );

      if (attachedAction) {
        networkFee = attachedAction.networkFee;
      }

      return {
        ...action,
        token: null,
        amount,
        networkFee,
      };
    });
};

Step 7. Go to amplify/backend/function/fetchDomainBalance/src/index.js and check inOutActions don't contain the regular actions (Simple payments or Transfer funds)

Resolves #3658

@mmioana mmioana self-assigned this Nov 7, 2024
@mmioana mmioana marked this pull request as ready for review November 7, 2024 09:09
@mmioana mmioana requested review from a team as code owners November 7, 2024 09:09
Fix: Fixes for fetchDomainBalance lambda edge cases
@mmioana mmioana force-pushed the fix/3658-fetch-domain-balance-edge-cases branch from aedb9c9 to f6ca765 Compare November 7, 2024 09:25
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Yup, the code changes look sane, I will absolutely trust your judgement here as the master of fetchDomainBalance lambda :D

  1. Still runs as it should ✔️
    image
  2. Negative and 0 don't bork ✔️
    image
    image
    Arbitrum balance ✔️
    image
    No simple payments or move funds:
    image
    image

const periodForTimeframe = getPeriodFor(
timeframePeriod,
timeframeType,
timeframePeriodEndDate,
);

console.log(
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional or a leftover 👀 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional, so we can get some more information if there is an error in a specific call

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected. Nice fix here! 👍

Screenshot from 2024-11-07 12-43-00
Screenshot from 2024-11-07 12-43-36
Screenshot from 2024-11-07 12-43-47
Screenshot from 2024-11-07 12-44-25
Screenshot from 2024-11-07 12-45-14
Screenshot from 2024-11-07 12-46-35

Copy link
Contributor

@rumzledz rumzledz left a comment

Choose a reason for hiding this comment

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

Can confirm testing steps are telling me the truth and nothing but the truth! 🙌

GraphQL Queries

timeFramePeriod = 1

Screenshot 2024-11-07 at 19 36 04

timeFramePeriod = -10

Screenshot 2024-11-07 at 19 36 26

timeFramePeriod = 21

Screenshot 2024-11-07 at 19 36 32

Code changes

network = arbitrumOne

Screenshot 2024-11-07 at 19 36 53

returning token: null from getFormattedOptions()

Screenshot 2024-11-07 at 19 37 49

Results

Before After (no more simple payment types)
Screenshot 2024-11-07 at 19 41 41 Screenshot 2024-11-07 at 19 40 50

@mmioana mmioana merged commit 97e16e9 into master Nov 8, 2024
4 of 6 checks passed
@mmioana mmioana deleted the fix/3658-fetch-domain-balance-edge-cases branch November 8, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QA Dashboard: Fetch domain balance lambda fixes for edge cases
4 participants