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

feat: announcement banner on deposit withdraw screens #297

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kylehoang92
Copy link
Collaborator

No description provided.

@kylehoang92 kylehoang92 force-pushed the feat/announcement-banner-on-deposit-withdraw-screens branch 5 times, most recently from 3c89724 to 3b5f7b9 Compare December 18, 2024 07:16
@kylehoang92 kylehoang92 force-pushed the feat/announcement-banner-on-deposit-withdraw-screens branch from 3b5f7b9 to 184f8df Compare December 18, 2024 08:17
@kylehoang92 kylehoang92 changed the title Feat/announcement banner on deposit withdraw screens feat: announcement banner on deposit withdraw screens Dec 18, 2024
@@ -516,6 +516,61 @@
}
}
},
"transfer_banner": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe can rename this field disabled_transfer_banner_config for more clarity.

## TransferBanner Data Structure
|Field |Type |Required |Description |Notes |
|---|---|---|---|---|
|`no_longer_supported_tokens` |`string[]` |false |List of tokens are no longer supported |
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Please rename this field to unsupported_tokens
  2. Please revise this description to List of tokens that are no longer supported
  3. Please add a note stating that The token denoms listed here **MUST** match the token denoms listed under the Carbon [Tokens API](https://api.carbon.network/carbon/coin/v1/tokens?pagination.limit=10000)..

|Field |Type |Required |Description |Notes |
|---|---|---|---|---|
|`no_longer_supported_tokens` |`string[]` |false |List of tokens are no longer supported |
|`temporary_disabled_transfer_tokens` |`object` |false |List of tokens for which deposits and withdrawals have been temporarily disabled |
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Pls rename this field to temp_disabled_transfer_tokens
  2. Please add a note that The token denoms listed in this object **MUST** match the token denoms listed under the Carbon [Tokens API](https://api.carbon.network/carbon/coin/v1/tokens?pagination.limit=10000).

|---|---|---|---|---|
|`no_longer_supported_tokens` |`string[]` |false |List of tokens are no longer supported |
|`temporary_disabled_transfer_tokens` |`object` |false |List of tokens for which deposits and withdrawals have been temporarily disabled |
|`temporary_disabled_bridges` |`object` |false |List of bridges for which deposits and withdrawals have been temporarily disabled |
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Pls rename this field to temp_disabled_bridges
  2. Please add a note that Blockchain network listed here **MUST** match the valid chainName of the bridges listed under BridgeAll RPC call.<br /><br /> To view the values of BridgeAll RPC call, simply run yarn get-bridges [network]on the command line. Sample for mainnet:yarn get-bridges mainnet``

"required": [
"start",
"end"
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think instead of assigning start and end to empty strings, would be good to make both fields optional, then you just need to make it an empty object.
i.e.

{
  "temporary_disabled_transfer_tokens": {
    "token_1": {}
  }
}

@@ -426,6 +443,35 @@ function isValidMarketPromo(marketPromo: {[marketId: string]: MarketPromo}, netw
return true;
}

function isValidTransferBanner(transferBanner: TransferBanner, network: CarbonSDK.Network): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a check for no_longer_supported_tokens to make sure that all the token denoms are valid. Please refer to this code snippet to see how to check if denoms are valid.

function isValidTransferBanner(transferBanner: TransferBanner, network: CarbonSDK.Network): boolean {
const { temporary_disabled_transfer_tokens, temporary_disabled_bridges } = transferBanner;
if (Object.keys(temporary_disabled_transfer_tokens).length > 0) {
Object.keys(temporary_disabled_transfer_tokens).map((key) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I see Object.keys(temporary_disabled_transfer_tokens) is used twice, pls store this in a variable and apply where necessary.
    i.e.
const disabledTokenKeys = Object.keys(temporary_disabled_transfer_tokens)
if (disabledTokenKeys.length > 0) {
  disabledTokenKeys.forEach((key) => { ... })
}
  1. Pls check that each denom key is valid. Please refer to this code snippet to see how to check if denoms are valid.

const startTime = new Date(start);
const endTime = new Date(end);
if (endTime < startTime) {
console.error(`ERROR: ${network}.json has invalid end time (${end}) is before start time (${start}) for token denom ${key}.`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls revise the message to ERROR: ${network}.json has an invalid end time (${end}) as it is before start time (${start}) for token denom ${key}.

}

if (Object.keys(temporary_disabled_bridges).length > 0) {
Object.keys(temporary_disabled_bridges).map((key) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I see Object.keys(temporary_disabled_bridges) is used twice, pls store this in a variable and apply where necessary.
    i.e.
const disabledBridgesKeys = Object.keys(temporary_disabled_bridges)
if (disabledBridgesKeys.length > 0) {
  disabledBridgesKeys.forEach((key) => { ... })
}
  1. Pls check that each bridge key is valid. Please refer to this code snippet to see how to check if the bridge names are valid.

const startTime = new Date(start);
const endTime = new Date(end);
if (endTime < startTime) {
console.error(`ERROR: ${network}.json has invalid end time (${end}) is before start time (${start}) for bridge address ${key}.`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls revise the message to ERROR: ${network}.json has an invalid end time (${end}) as it is before start time (${start}) for bridge ${key}.

@sarah-thong
Copy link
Collaborator

Other Comments

  • Don't we already have another field transfer_disabled_tokens to temporarily disable token transfers. Pls choose 1 and remove the other 1 for this PR.

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.

2 participants