Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

Implemented KAS repoistory to load contracts data #588

Merged

Conversation

jeongkyun-oh
Copy link
Contributor

Proposed changes

  • Added insert contract method in KAS repository.
  • Introduced contractCaller in order to check if the deployed contracts supports KIP series.
  • This is imported from kas-app-server.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

@ehnuje ehnuje left a comment

Choose a reason for hiding this comment

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

Please make tests for possible cases.

datasync/chaindatafetcher/kas/repository_contracts.go Outdated Show resolved Hide resolved
datasync/chaindatafetcher/kas/repository_contracts.go Outdated Show resolved Hide resolved
Copy link
Contributor

@KimKyungup KimKyungup left a comment

Choose a reason for hiding this comment

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

LGTM

@jeongkyun-oh jeongkyun-oh force-pushed the PDK-1561-repository-impl-contracts branch from 692dfe6 to 1e8e379 Compare August 6, 2020 06:26
KimKyungup
KimKyungup previously approved these changes Aug 6, 2020
@jeongkyun-oh jeongkyun-oh force-pushed the PDK-1561-repository-impl-contracts branch from b4b0cc2 to 950fa34 Compare August 7, 2020 06:00
KimKyungup
KimKyungup previously approved these changes Aug 7, 2020
ehnuje
ehnuje previously approved these changes Aug 7, 2020
Comment on lines +130 to +135
if isInvalid, err := f.supportsInterface(contract, opts, InvalidId); err != nil {
logger.Error("supportsInterface is failed", "contract", contract.String(), "blockNumber", blockNumber, "interfaceID", hexutil.Encode(InvalidId[:]))
return false, err
} else if isInvalid {
return false, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Is checking for InvalidId mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, according to the KIP-13 specification, it is required.
ref: https://kips.klaytn.com/KIPs/kip-13#how-to-query-if-a-contract-implements-kip-13

@aidan-kwon
Copy link
Member

Please, consider another way of executing a contract without PublicBlockChainAPI

aidan-kwon
aidan-kwon previously approved these changes Aug 7, 2020
@jeongkyun-oh jeongkyun-oh force-pushed the PDK-1561-repository-impl-contracts branch from 6fe2202 to 6f0026f Compare August 7, 2020 09:33
@jeongkyun-oh
Copy link
Contributor Author

@aidan-kwon Under the circumstance, I will consider PublicBlockChainAPI issue later on. I will make a JIRA ticket to follow up.

@jeongkyun-oh
Copy link
Contributor Author

PTAL @KimKyungup @aidan-kwon @ehnuje

KimKyungup
KimKyungup previously approved these changes Aug 10, 2020
aidan-kwon
aidan-kwon previously approved these changes Aug 10, 2020
@jeongkyun-oh jeongkyun-oh force-pushed the PDK-1561-repository-impl-contracts branch from 6f0026f to cbbc865 Compare August 10, 2020 03:57
@jeongkyun-oh jeongkyun-oh force-pushed the PDK-1561-repository-impl-contracts branch from 8e5950d to e101e5c Compare August 10, 2020 04:07
@jeongkyun-oh
Copy link
Contributor Author

Sorry to bother you, but I resolved conflicts. Please take another look. @ehnuje @KimKyungup

@jeongkyun-oh jeongkyun-oh merged commit da7babb into klaytn:dev Aug 10, 2020
@jeongkyun-oh jeongkyun-oh deleted the PDK-1561-repository-impl-contracts branch August 10, 2020 04:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants